diff --git a/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java b/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java index 98aa82fcb5..bbaa15287e 100644 --- a/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java +++ b/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java @@ -24,6 +24,7 @@ import java.util.Arrays; import java.util.IdentityHashMap; import java.util.LinkedList; import java.util.List; +import java.util.HashSet; import java.util.Map; import java.util.TreeSet; import java.util.concurrent.CountDownLatch; @@ -50,8 +51,8 @@ public class PeerConnectionTest extends ActivityTestCase { private int expectedIceCandidates = 0; private int expectedErrors = 0; private int expectedRenegotiations = 0; - private int previouslySeenWidth = 0; - private int previouslySeenHeight = 0; + private int expectedWidth = 0; + private int expectedHeight = 0; private int expectedFramesDelivered = 0; private LinkedList expectedSignalingChanges = new LinkedList(); @@ -77,6 +78,8 @@ public class PeerConnectionTest extends ActivityTestCase { private int expectedStatsCallbacks = 0; private LinkedList gotStatsReports = new LinkedList(); + private final HashSet gotRemoteStreams = + new HashSet(); public ObserverExpectations(String name) { this.name = name; @@ -106,18 +109,9 @@ public class PeerConnectionTest extends ActivityTestCase { } } - private synchronized void setSize(int width, int height) { - // Because different camera devices (fake & physical) produce different - // resolutions, we only sanity-check the set sizes, - assertTrue(width > 0); - assertTrue(height > 0); - if (previouslySeenWidth > 0) { - assertEquals(previouslySeenWidth, width); - assertEquals(previouslySeenHeight, height); - } else { - previouslySeenWidth = width; - previouslySeenHeight = height; - } + public synchronized void setExpectedResolution(int width, int height) { + expectedWidth = width; + expectedHeight = height; } public synchronized void expectFramesDelivered(int count) { @@ -126,7 +120,10 @@ public class PeerConnectionTest extends ActivityTestCase { @Override public synchronized void renderFrame(VideoRenderer.I420Frame frame) { - setSize(frame.rotatedWidth(), frame.rotatedHeight()); + assertTrue(expectedWidth > 0); + assertTrue(expectedHeight > 0); + assertEquals(expectedWidth, frame.rotatedWidth()); + assertEquals(expectedHeight, frame.rotatedHeight()); --expectedFramesDelivered; VideoRenderer.renderFrameDone(frame); } @@ -200,6 +197,7 @@ public class PeerConnectionTest extends ActivityTestCase { stream.videoTracks.get(0).addRenderer(renderer); assertNull(renderers.put( stream, new WeakReference(renderer))); + gotRemoteStreams.add(stream); } public synchronized void expectRemoveStream(String label) { @@ -214,6 +212,7 @@ public class PeerConnectionTest extends ActivityTestCase { assertNotNull(renderer.get()); assertEquals(1, stream.videoTracks.size()); stream.videoTracks.get(0).removeRenderer(renderer.get()); + gotRemoteStreams.remove(stream); } public synchronized void expectDataChannel(String label) { @@ -388,6 +387,25 @@ public class PeerConnectionTest extends ActivityTestCase { } } + // Sets the expected resolution for an ObserverExpectations once a frame + // has been captured. + private static class ExpectedResolutionSetter implements VideoRenderer.Callbacks { + private ObserverExpectations observer; + + public ExpectedResolutionSetter(ObserverExpectations observer) { + this.observer = observer; + } + + @Override + public synchronized void renderFrame(VideoRenderer.I420Frame frame) { + // Because different camera devices (fake & physical) produce different + // resolutions, we only sanity-check the set sizes, + assertTrue(frame.rotatedWidth() > 0); + assertTrue(frame.rotatedHeight() > 0); + observer.setExpectedResolution(frame.rotatedWidth(), frame.rotatedHeight()); + } + } + private static class SdpObserverLatch implements SdpObserver { private boolean success = false; private SessionDescription sdp = null; @@ -496,6 +514,17 @@ public class PeerConnectionTest extends ActivityTestCase { // Thread.sleep(100); } + // TODO(fischman) MOAR test ideas: + // - Test that PC.removeStream() works; requires a second + // createOffer/createAnswer dance. + // - audit each place that uses |constraints| for specifying non-trivial + // constraints (and ensure they're honored). + // - test error cases + // - ensure reasonable coverage of _jni.cc is achieved. Coverage is + // extra-important because of all the free-text (class/method names, etc) + // in JNI-style programming; make sure no typos! + // - Test that shutdown mid-interaction is crash-free. + @MediumTest public void testCompleteSession() throws Exception { // Allow loopback interfaces too since our Android devices often don't @@ -540,7 +569,8 @@ public class PeerConnectionTest extends ActivityTestCase { offeringExpectations.expectRenegotiationNeeded(); WeakReference oLMS = addTracksToPC( factory, offeringPC, videoSource, "offeredMediaStream", - "offeredVideoTrack", "offeredAudioTrack", offeringExpectations); + "offeredVideoTrack", "offeredAudioTrack", + new ExpectedResolutionSetter(answeringExpectations)); offeringExpectations.expectRenegotiationNeeded(); DataChannel offeringDC = offeringPC.createDataChannel( @@ -571,7 +601,8 @@ public class PeerConnectionTest extends ActivityTestCase { answeringExpectations.expectRenegotiationNeeded(); WeakReference aLMS = addTracksToPC( factory, answeringPC, videoSource, "answeredMediaStream", - "answeredVideoTrack", "answeredAudioTrack", answeringExpectations); + "answeredVideoTrack", "answeredAudioTrack", + new ExpectedResolutionSetter(offeringExpectations)); sdpLatch = new SdpObserverLatch(); answeringPC.createAnswer(sdpLatch, new MediaConstraints()); @@ -688,17 +719,6 @@ public class PeerConnectionTest extends ActivityTestCase { answeringExpectations.dataChannel.close(); offeringExpectations.dataChannel.close(); - // TODO(fischman) MOAR test ideas: - // - Test that PC.removeStream() works; requires a second - // createOffer/createAnswer dance. - // - audit each place that uses |constraints| for specifying non-trivial - // constraints (and ensure they're honored). - // - test error cases - // - ensure reasonable coverage of _jni.cc is achieved. Coverage is - // extra-important because of all the free-text (class/method names, etc) - // in JNI-style programming; make sure no typos! - // - Test that shutdown mid-interaction is crash-free. - // Free the Java-land objects, collect them, and sleep a bit to make sure we // don't get late-arrival crashes after the Java-land objects have been // freed. @@ -711,10 +731,226 @@ public class PeerConnectionTest extends ActivityTestCase { System.gc(); } + @MediumTest + public void testTrackRemoval() throws Exception { + // Allow loopback interfaces too since our Android devices often don't + // have those. + PeerConnectionFactory.Options options = new PeerConnectionFactory.Options(); + options.networkIgnoreMask = 0; + PeerConnectionFactory factory = new PeerConnectionFactory(options); + + MediaConstraints pcConstraints = new MediaConstraints(); + pcConstraints.mandatory.add( + new MediaConstraints.KeyValuePair("DtlsSrtpKeyAgreement", "true")); + + LinkedList iceServers = + new LinkedList(); + iceServers.add(new PeerConnection.IceServer( + "stun:stun.l.google.com:19302")); + + ObserverExpectations offeringExpectations = + new ObserverExpectations("PCTest:offerer"); + PeerConnection offeringPC = factory.createPeerConnection( + iceServers, pcConstraints, offeringExpectations); + assertNotNull(offeringPC); + + ObserverExpectations answeringExpectations = + new ObserverExpectations("PCTest:answerer"); + PeerConnection answeringPC = factory.createPeerConnection( + iceServers, pcConstraints, answeringExpectations); + assertNotNull(answeringPC); + + // We want to use the same camera for offerer & answerer, so create it here + // instead of in addTracksToPC. + VideoSource videoSource = factory.createVideoSource( + VideoCapturerAndroid.create("", null), new MediaConstraints()); + + // Add offerer media stream. + offeringExpectations.expectRenegotiationNeeded(); + WeakReference oLMS = addTracksToPC( + factory, offeringPC, videoSource, "offeredMediaStream", + "offeredVideoTrack", "offeredAudioTrack", + new ExpectedResolutionSetter(answeringExpectations)); + + // Create offer. + SdpObserverLatch sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + SessionDescription offerSdp = sdpLatch.getSdp(); + assertEquals(offerSdp.type, SessionDescription.Type.OFFER); + assertFalse(offerSdp.description.isEmpty()); + + // Set local description for offerer. + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER); + offeringExpectations.expectIceCandidates(2); + offeringExpectations.expectIceGatheringChange(IceGatheringState.COMPLETE); + offeringPC.setLocalDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Set remote description for answerer. + sdpLatch = new SdpObserverLatch(); + answeringExpectations.expectSignalingChange( + SignalingState.HAVE_REMOTE_OFFER); + answeringExpectations.expectAddStream("offeredMediaStream"); + answeringPC.setRemoteDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Add answerer media stream. + answeringExpectations.expectRenegotiationNeeded(); + WeakReference aLMS = addTracksToPC( + factory, answeringPC, videoSource, "answeredMediaStream", + "answeredVideoTrack", "answeredAudioTrack", + new ExpectedResolutionSetter(offeringExpectations)); + + // Create answer. + sdpLatch = new SdpObserverLatch(); + answeringPC.createAnswer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + SessionDescription answerSdp = sdpLatch.getSdp(); + assertEquals(answerSdp.type, SessionDescription.Type.ANSWER); + assertFalse(answerSdp.description.isEmpty()); + + // Set local description for answerer. + sdpLatch = new SdpObserverLatch(); + answeringExpectations.expectSignalingChange(SignalingState.STABLE); + answeringExpectations.expectIceCandidates(2); + answeringExpectations.expectIceGatheringChange(IceGatheringState.COMPLETE); + answeringPC.setLocalDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Set remote description for offerer. + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.STABLE); + offeringExpectations.expectAddStream("answeredMediaStream"); + + offeringExpectations.expectIceConnectionChange( + IceConnectionState.CHECKING); + offeringExpectations.expectIceConnectionChange( + IceConnectionState.CONNECTED); + // TODO(bemasc): uncomment once delivery of ICECompleted is reliable + // (https://code.google.com/p/webrtc/issues/detail?id=3021). + // + // offeringExpectations.expectIceConnectionChange( + // IceConnectionState.COMPLETED); + answeringExpectations.expectIceConnectionChange( + IceConnectionState.CHECKING); + answeringExpectations.expectIceConnectionChange( + IceConnectionState.CONNECTED); + + offeringPC.setRemoteDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Wait for at least one ice candidate from the offering PC and forward them to the answering + // PC. + for (IceCandidate candidate : offeringExpectations.getAtLeastOneIceCandidate()) { + answeringPC.addIceCandidate(candidate); + } + + // Wait for at least one ice candidate from the answering PC and forward them to the offering + // PC. + for (IceCandidate candidate : answeringExpectations.getAtLeastOneIceCandidate()) { + offeringPC.addIceCandidate(candidate); + } + + // Wait for one frame of the correct size to be delivered. + // Otherwise we could get a dummy black frame of unexpcted size when the + // video track is removed. + offeringExpectations.expectFramesDelivered(1); + answeringExpectations.expectFramesDelivered(1); + + assertTrue(offeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + assertTrue(answeringExpectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); + + assertEquals( + PeerConnection.SignalingState.STABLE, offeringPC.signalingState()); + assertEquals( + PeerConnection.SignalingState.STABLE, answeringPC.signalingState()); + + // Now do another negotiation, removing the video track from one peer. + // This previously caused a crash on pc.dispose(). + // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5128 + VideoTrack offererVideoTrack = oLMS.get().videoTracks.get(0); + // Note that when we call removeTrack, we regain responsibility for + // disposing of the track. + oLMS.get().removeTrack(offererVideoTrack); + + // Create offer. + sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + offerSdp = sdpLatch.getSdp(); + assertEquals(offerSdp.type, SessionDescription.Type.OFFER); + assertFalse(offerSdp.description.isEmpty()); + + // Set local description for offerer. + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER); + offeringPC.setLocalDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Set remote description for answerer. + sdpLatch = new SdpObserverLatch(); + answeringExpectations.expectSignalingChange( + SignalingState.HAVE_REMOTE_OFFER); + answeringPC.setRemoteDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Create answer. + sdpLatch = new SdpObserverLatch(); + answeringPC.createAnswer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + answerSdp = sdpLatch.getSdp(); + assertEquals(answerSdp.type, SessionDescription.Type.ANSWER); + assertFalse(answerSdp.description.isEmpty()); + + // Set local description for answerer. + sdpLatch = new SdpObserverLatch(); + answeringExpectations.expectSignalingChange(SignalingState.STABLE); + answeringPC.setLocalDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Set remote description for offerer. + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.STABLE); + offeringPC.setRemoteDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Make sure the track was really removed. + // TODO(deadbeef): Currently the expectation is that the video track's + // state will be set to "ended". However, in the future, it's likely that + // the video track will be completely removed from the remote stream + // (as it is on the C++ level). + MediaStream aRMS = answeringExpectations.gotRemoteStreams.iterator().next(); + assertEquals(aRMS.videoTracks.get(0).state(), MediaStreamTrack.State.ENDED); + + // Free the Java-land objects, collect them, and sleep a bit to make sure we + // don't get late-arrival crashes after the Java-land objects have been + // freed. + shutdownPC(offeringPC, offeringExpectations); + offeringPC = null; + shutdownPC(answeringPC, answeringExpectations); + answeringPC = null; + offererVideoTrack.dispose(); + videoSource.dispose(); + factory.dispose(); + System.gc(); + } + private static void shutdownPC( PeerConnection pc, ObserverExpectations expectations) { - expectations.dataChannel.unregisterObserver(); - expectations.dataChannel.dispose(); + if (expectations.dataChannel != null) { + expectations.dataChannel.unregisterObserver(); + expectations.dataChannel.dispose(); + } expectations.expectStatsCallback(); assertTrue(pc.getStats(expectations, null)); assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); diff --git a/webrtc/api/java/src/org/webrtc/MediaStream.java b/webrtc/api/java/src/org/webrtc/MediaStream.java index d18c72cca3..2128b735bf 100644 --- a/webrtc/api/java/src/org/webrtc/MediaStream.java +++ b/webrtc/api/java/src/org/webrtc/MediaStream.java @@ -55,20 +55,14 @@ public class MediaStream { } public boolean removeTrack(AudioTrack track) { - if (nativeRemoveAudioTrack(nativeStream, track.nativeTrack)) { - audioTracks.remove(track); - return true; - } - return false; + audioTracks.remove(track); + return nativeRemoveAudioTrack(nativeStream, track.nativeTrack); } public boolean removeTrack(VideoTrack track) { - if (nativeRemoveVideoTrack(nativeStream, track.nativeTrack)) { - videoTracks.remove(track); - preservedVideoTracks.remove(track); - return true; - } - return false; + videoTracks.remove(track); + preservedVideoTracks.remove(track); + return nativeRemoveVideoTrack(nativeStream, track.nativeTrack); } public void dispose() { diff --git a/webrtc/media/webrtc/webrtcvideoengine2.cc b/webrtc/media/webrtc/webrtcvideoengine2.cc index fc22e9ccb6..0a7a27bb47 100644 --- a/webrtc/media/webrtc/webrtcvideoengine2.cc +++ b/webrtc/media/webrtc/webrtcvideoengine2.cc @@ -1544,7 +1544,8 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::~WebRtcVideoSendStream() { static void CreateBlackFrame(webrtc::VideoFrame* video_frame, int width, - int height) { + int height, + webrtc::VideoRotation rotation) { video_frame->CreateEmptyFrame(width, height, width, (width + 1) / 2, (width + 1) / 2); memset(video_frame->buffer(webrtc::kYPlane), 16, @@ -1553,6 +1554,7 @@ static void CreateBlackFrame(webrtc::VideoFrame* video_frame, video_frame->allocated_size(webrtc::kUPlane)); memset(video_frame->buffer(webrtc::kVPlane), 128, video_frame->allocated_size(webrtc::kVPlane)); + video_frame->set_rotation(rotation); } void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( @@ -1574,9 +1576,9 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( if (muted_) { // Create a black frame to transmit instead. - CreateBlackFrame(&video_frame, - static_cast(frame->GetWidth()), - static_cast(frame->GetHeight())); + CreateBlackFrame(&video_frame, static_cast(frame->GetWidth()), + static_cast(frame->GetHeight()), + frame->GetVideoRotation()); } int64_t frame_delta_ms = frame->GetTimeStamp() / rtc::kNumNanosecsPerMillisec; @@ -1590,6 +1592,7 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( // Reconfigure codec if necessary. SetDimensions( video_frame.width(), video_frame.height(), capturer->IsScreencast()); + last_rotation_ = video_frame.rotation(); stream_->Input()->IncomingCapturedFrame(video_frame); } @@ -1614,7 +1617,7 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer( webrtc::VideoFrame black_frame; CreateBlackFrame(&black_frame, last_dimensions_.width, - last_dimensions_.height); + last_dimensions_.height, last_rotation_); // Force this black frame not to be dropped due to timestamp order // check. As IncomingCapturedFrame will drop the frame if this frame's diff --git a/webrtc/media/webrtc/webrtcvideoengine2.h b/webrtc/media/webrtc/webrtcvideoengine2.h index 98bad0f007..27eec863ed 100644 --- a/webrtc/media/webrtc/webrtcvideoengine2.h +++ b/webrtc/media/webrtc/webrtcvideoengine2.h @@ -345,6 +345,8 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, VideoEncoderSettings encoder_settings_ GUARDED_BY(lock_); AllocatedEncoder allocated_encoder_ GUARDED_BY(lock_); Dimensions last_dimensions_ GUARDED_BY(lock_); + webrtc::VideoRotation last_rotation_ GUARDED_BY(lock_) = + webrtc::kVideoRotation_0; VideoCapturer* capturer_ GUARDED_BY(lock_); bool sending_ GUARDED_BY(lock_);