Fixing bug in MediaStream.java that caused double disposal of track.

Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test.
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where generated
black frames don't get any rotation.

BUG=webrtc:5128
Committed: https://crrev.com/6ecee07bab2ad781de8877d6d40c8f540973170d
Cr-Commit-Position: refs/heads/master@{#11583}

[Taylor Brandstetter, 2016-01-26 21:52:14]

deadbeef@webrtc.org changed reviewers:
+ glaznev@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-01-26 21:52:14]

PTAL. The issue is that if removeTrack fails, the track is kept in the list, so
it gets disposed of repeatedly until something crashes.

[AlexG, 2016-01-26 22:03:36]

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/MediaStream.java (right):

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaStream.java:96:
audioTracks.remove(track);
May be call audioTracks.remove() from removeTrack() when
nativeRemoveAudioTrack() fails to keep logic in a single place?

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaStream.java:103:
videoTracks.remove(track);
ditto

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/testcomm...
File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
(right):

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/testcomm...
talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:724: //
Do another negotiation, removing the video track from one peer.
I think it should be under the boolean flag in doTest() and moved to separate
function - excersiceTrackRemoval()?
doTest() is already too big.

[Taylor Brandstetter, 2016-01-26 22:43:10]

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/MediaStream.java (right):

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaStream.java:96:
audioTracks.remove(track);
On 2016/01/26 22:03:36, AlexG wrote:
> May be call audioTracks.remove() from removeTrack() when
> nativeRemoveAudioTrack() fails to keep logic in a single place?

I thought maybe there was some motivation for not removing the track in
removeTrack, so I tried to confine my changes to dispose(). But if you can't
think of any reason, I'll go with your suggestion.

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/testcomm...
File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
(right):

https://codereview.webrtc.org/1639583003/diff/1/talk/app/webrtc/java/testcomm...
talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:724: //
Do another negotiation, removing the video track from one peer.
On 2016/01/26 22:03:36, AlexG wrote:
> I think it should be under the boolean flag in doTest() and moved to separate
> function - excersiceTrackRemoval()?
> doTest() is already too big.

I just went ahead and created a separate test method.

[AlexG, 2016-01-26 23:03:55]

lgtm

https://codereview.webrtc.org/1639583003/diff/40001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaStream.java (right):

https://codereview.webrtc.org/1639583003/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaStream.java:75: boolean ret =
nativeRemoveAudioTrack(nativeStream, track.nativeTrack);
nit: a little shorter:
audioTracks.remove(track)
return  nativeRemoveAudioTrack(...)

[Taylor Brandstetter, 2016-01-27 17:36:12]

https://codereview.webrtc.org/1639583003/diff/40001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaStream.java (right):

https://codereview.webrtc.org/1639583003/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaStream.java:75: boolean ret =
nativeRemoveAudioTrack(nativeStream, track.nativeTrack);
On 2016/01/26 23:03:55, AlexG wrote:
> nit: a little shorter:
> audioTracks.remove(track)
> return  nativeRemoveAudioTrack(...)

Done.

[Taylor Brandstetter, 2016-01-27 17:36:29]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-01-27 17:36:29]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1639583003/#ps60001 (title:
"Making removeTrack even simpler")

[commit-bot: I haz the power, 2016-01-27 17:36:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1639583003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1639583003/60001

[commit-bot: I haz the power, 2016-01-27 18:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 18:06:25]

Try jobs failed on following builders:
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[Taylor Brandstetter, 2016-01-28 21:00:31]

Description was changed from

==========
Fixing bug in MediaStream.java that caused double disposal of track.

BUG=webrtc:5128
==========

to

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. It wasn't correctly
waiting for 10 video frames to be received.

BUG=webrtc:5128
==========

[Taylor Brandstetter, 2016-01-28 21:03:44]

I made some changes to the unit test; could you review again?

An issue I found is that the same class is being used for both the local and
remote renderer callbacks. So the expectation "10 frames delivered" could be
satisfied when 5 frames are captured and 5 are delivered. Or worse, when 10 are
captured and 0 are delivered.

So, I created an "ExpectedResolutionSetter" class that listens to the local
render callbacks, purely to set the expected resolution for the PeerConnection
on the other side.

[AlexG, 2016-02-01 20:50:15]

On 2016/01/28 21:03:44, Taylor Brandstetter wrote:
> I made some changes to the unit test; could you review again?
> 
> An issue I found is that the same class is being used for both the local and
> remote renderer callbacks. So the expectation "10 frames delivered" could be
> satisfied when 5 frames are captured and 5 are delivered. Or worse, when 10
are
> captured and 0 are delivered.
> 
> So, I created an "ExpectedResolutionSetter" class that listens to the local
> render callbacks, purely to set the expected resolution for the PeerConnection
> on the other side.

I think this may be done in a same way as Android test - 
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/examples/an...
using 
private static class MockRenderer implements VideoRenderer.Callbacks { .. }
and creating two separate instances for local and remote renderer.

But I think this test will no longer be relevant soon - please correct me if I
am wrong but I think this is running on Linux platform only which will be
abandoned soon

https://codereview.webrtc.org/1652123002/

May be it is possible to land fixes in MediaStream.java and then consider adding
MediaStream track removal test to Android in a separate CL?

[AlexG, 2016-02-01 20:52:43]

On 2016/02/01 20:50:15, AlexG wrote:
> On 2016/01/28 21:03:44, Taylor Brandstetter wrote:
> > I made some changes to the unit test; could you review again?
> > 
> > An issue I found is that the same class is being used for both the local and
> > remote renderer callbacks. So the expectation "10 frames delivered" could be
> > satisfied when 5 frames are captured and 5 are delivered. Or worse, when 10
> are
> > captured and 0 are delivered.
> > 
> > So, I created an "ExpectedResolutionSetter" class that listens to the local
> > render callbacks, purely to set the expected resolution for the
PeerConnection
> > on the other side.
> 
> I think this may be done in a same way as Android test - 
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/examples/an...
> using 
> private static class MockRenderer implements VideoRenderer.Callbacks { .. }
> and creating two separate instances for local and remote renderer.
> 
> But I think this test will no longer be relevant soon - please correct me if I
> am wrong but I think this is running on Linux platform only which will be
> abandoned soon
> 
> https://codereview.webrtc.org/1652123002/
> 
> May be it is possible to land fixes in MediaStream.java and then consider
adding
> MediaStream track removal test to Android in a separate CL?

I am also ok with landing this CL now as is, but your changes in test will soon
be deleted :)

[Taylor Brandstetter, 2016-02-08 19:48:09]

I rebased after the test was moved (thanks for the heads up, Alex).

pbos: Could you review the webrtcvideoengine2 changes? The issue I found was
that the black frames (created when the track is muted, or when the capturer is
set to null) don't get the correct rotation. The new test I added - which ends
up setting the capturer to null - revealed this issue.

[Taylor Brandstetter, 2016-02-08 19:49:06]

Description was changed from

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. It wasn't correctly
waiting for 10 video frames to be received.

BUG=webrtc:5128
==========

to

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. 
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where when a black
frame is generated, it doesn't get any rotation.

BUG=webrtc:5128
==========

[Taylor Brandstetter, 2016-02-08 19:49:41]

Description was changed from

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. 
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where when a black
frame is generated, it doesn't get any rotation.

BUG=webrtc:5128
==========

to

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. 
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where generated
black frames don't get any rotation.

BUG=webrtc:5128
==========

[Taylor Brandstetter, 2016-02-08 20:45:14]

deadbeef@webrtc.org changed reviewers:
+ pbos@webrtc.org

[Taylor Brandstetter, 2016-02-08 20:45:15]

Forgot to add pbos. See: https://codereview.webrtc.org/1639583003/#msg15

[pbos-webrtc, 2016-02-10 19:17:59]

On 2016/02/08 20:45:15, Taylor Brandstetter wrote:
> Forgot to add pbos. See: https://codereview.webrtc.org/1639583003/#msg15

I assume this might have been to perkj@, is it still relevant?

lgtm for webrtc/media/webrtc/

[Taylor Brandstetter, 2016-02-10 19:44:56]

On 2016/02/10 19:17:59, pbos-webrtc wrote:
> On 2016/02/08 20:45:15, Taylor Brandstetter wrote:
> > Forgot to add pbos. See: https://codereview.webrtc.org/1639583003/#msg15
> 
> I assume this might have been to perkj@, is it still relevant?
> 
> lgtm for webrtc/media/webrtc/

Is perkj a better webrtcvideoengine2 owner? He's not in webrtc/media/OWNERS
though. Must be a consequence of the talk->webrtc move, since he was a "talk"
owner but isn't a "webrtc" owner.

[pbos-webrtc, 2016-02-10 20:04:13]

On 2016/02/10 19:44:56, Taylor Brandstetter wrote:
> On 2016/02/10 19:17:59, pbos-webrtc wrote:
> > On 2016/02/08 20:45:15, Taylor Brandstetter wrote:
> > > Forgot to add pbos. See: https://codereview.webrtc.org/1639583003/#msg15
> > 
> > I assume this might have been to perkj@, is it still relevant?
> > 
> > lgtm for webrtc/media/webrtc/
> 
> Is perkj a better webrtcvideoengine2 owner? He's not in webrtc/media/OWNERS
> though. Must be a consequence of the talk->webrtc move, since he was a "talk"
> owner but isn't a "webrtc" owner.

No I thought this was for the test in relation to the PC removal. I didn't see
what targeted me specifically in #msg15.

[Taylor Brandstetter, 2016-02-10 20:07:37]

On 2016/02/10 20:04:13, pbos-webrtc wrote:
> On 2016/02/10 19:44:56, Taylor Brandstetter wrote:
> > On 2016/02/10 19:17:59, pbos-webrtc wrote:
> > > On 2016/02/08 20:45:15, Taylor Brandstetter wrote:
> > > > Forgot to add pbos. See: https://codereview.webrtc.org/1639583003/#msg15
> > > 
> > > I assume this might have been to perkj@, is it still relevant?
> > > 
> > > lgtm for webrtc/media/webrtc/
> > 
> > Is perkj a better webrtcvideoengine2 owner? He's not in webrtc/media/OWNERS
> > though. Must be a consequence of the talk->webrtc move, since he was a
"talk"
> > owner but isn't a "webrtc" owner.
> 
> No I thought this was for the test in relation to the PC removal. I didn't see
> what targeted me specifically in #msg15.

Whoops, meant #msg16.

[pbos-webrtc, 2016-02-10 20:15:29]

On 2016/02/10 20:07:37, Taylor Brandstetter wrote:
> On 2016/02/10 20:04:13, pbos-webrtc wrote:
> > On 2016/02/10 19:44:56, Taylor Brandstetter wrote:
> > > On 2016/02/10 19:17:59, pbos-webrtc wrote:
> > > > On 2016/02/08 20:45:15, Taylor Brandstetter wrote:
> > > > > Forgot to add pbos. See:
https://codereview.webrtc.org/1639583003/#msg15
> > > > 
> > > > I assume this might have been to perkj@, is it still relevant?
> > > > 
> > > > lgtm for webrtc/media/webrtc/
> > > 
> > > Is perkj a better webrtcvideoengine2 owner? He's not in
webrtc/media/OWNERS
> > > though. Must be a consequence of the talk->webrtc move, since he was a
> "talk"
> > > owner but isn't a "webrtc" owner.
> > 
> > No I thought this was for the test in relation to the PC removal. I didn't
see
> > what targeted me specifically in #msg15.
> 
> Whoops, meant #msg16.

Then lgtm for that one. :)

[Taylor Brandstetter, 2016-02-10 21:21:49]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-02-10 21:21:50]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1639583003/#ps140001 (title:
"Merging with master due to move of talk directory.")

[commit-bot: I haz the power, 2016-02-10 21:21:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1639583003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1639583003/140001

[commit-bot: I haz the power, 2016-02-10 21:23:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-10 21:23:55]

Exceeded global retry quota

[Taylor Brandstetter, 2016-02-11 17:35:37]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-02-11 17:35:38]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1639583003/#ps160001 (title:
"Fixing placement of GUARDED_BY.")

[commit-bot: I haz the power, 2016-02-11 17:35:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1639583003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1639583003/160001

[commit-bot: I haz the power, 2016-02-11 17:57:26]

Description was changed from

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. 
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where generated
black frames don't get any rotation.

BUG=webrtc:5128
==========

to

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. 
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where generated
black frames don't get any rotation.

BUG=webrtc:5128
==========

[commit-bot: I haz the power, 2016-02-11 17:57:27]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-11 17:57:42]

Description was changed from

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test. 
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where generated
black frames don't get any rotation.

BUG=webrtc:5128
==========

to

==========
Fixing bug in MediaStream.java that caused double disposal of track.

Also fixing an issue with the Java PeerConnection unit test.
It wasn't correctly waiting for 10 video frames to be received.

And fixed an issue with the video engine, where generated
black frames don't get any rotation.

BUG=webrtc:5128

Committed: https://crrev.com/6ecee07bab2ad781de8877d6d40c8f540973170d
Cr-Commit-Position: refs/heads/master@{#11583}
==========

[commit-bot: I haz the power, 2016-02-11 17:57:43]

Patchset 9 (id:??) landed as
https://crrev.com/6ecee07bab2ad781de8877d6d40c8f540973170d
Cr-Commit-Position: refs/heads/master@{#11583}