Deliver video frames on Android, on the decode thread.

Deliver video frames on Android, on the decode thread.

VideoCoding
* Adding a method for polling for frames on Android only until the capture implementation takes care of this (longer term plan).

CodecDatabase
* Add an accessor for the current decoder
* Use std::unique_ptr<> for ownership.
* Remove "Release()" and "ReleaseDecoder()". Instead just delete.
* Remove |friend| relationship between CodecDatabase and VCMGenericDecoder.

VCMDecodedFrameCallback
* DCHECKs for thread correctness.
* Remove |lock_| now that a threading model has been established and verified.

VCMGenericDecoder
* All methods now have thread checks.
* Variable access associated with thread checkers.

VideoReceiver
* Added two notification methods, DecoderThreadStarting() and DecoderThreadStopped()
  * Allows us to establish a period when the decoder thread is not running and it is safe to modify variables such as callbacks, that are only read when the decoder thread is running.
  * Allows us to DCHECK thread guarantees.
  * Allows synchronizing callbacks from the module process thread and have them only active while the decoder thread is running.
  * The above, allows us to establish two modes for the thread, single-threaded-mutable and multi-threaded-const.
  * Using that knowledge, we can remove |receive_crit_| as well as locking for a number of member variables.

MediaCodecVideoDecoder
* Removed frame polling code from this class, since this is now done from the root thread function in VideoReceiveStream.

VideoReceiveStream
* On Android: Polls for decoded frames every 10ms (same interval as previously in MediaCodecVideoDecoder)
* [Un]Registers the |video_receiver_| with the module thread only around the time the decoder thread is started/stopped.
* Notifies the receiver of start/stop events of the decoder thread.
* Changed the decoder thread to use the new PlatformThread callback type.

BUG=webrtc:7361, 695438
Review-Url: https://codereview.webrtc.org/2764573002
Cr-Commit-Position: refs/heads/master@{#17527}
Committed: https://chromium.googlesource.com/external/webrtc/+/e3aa88bbd5accadec73fa7e38584dfbf6aabe8a9

[tommi, 2017-03-20 18:40:26]

tommi@webrtc.org changed reviewers:
+ mflodman@webrtc.org, sakal@webrtc.org, stefan@webrtc.org

[tommi, 2017-03-20 18:40:26]

sakal:
- webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc
(feel free to take a look at the rest though)

mflodman, stefan - I need one of you at least :)
- everthing else (and androidmediadecoder_jni.cc if you feel so inclined).

[tommi, 2017-03-20 18:40:33]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 18:40:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[tommi, 2017-03-20 18:43:40]

Description was changed from

==========
Deliver video frames on Android, on the decode thread.

VideoCoding
* Adding a method for polling for frames on Android only until the capture
implementation takes care of this (longer term plan).

CodecDatabase
* Add an accessor for the current decoder
* Use std::unique_ptr<> for ownership.
* Remove "Release()" and "ReleaseDecoder()". Instead just delete.
* Remove |friend| relationship between CodecDatabase and VCMGenericDecoder.

VCMDecodedFrameCallback
* DCHECKs for thread correctness.
* Remove |lock_| now that a threading model has been established and verified.

VCMGenericDecoder
* All methods now have thread checks.
* Variable access associated with thread checkers.

VideoReceiver
* Added two notification methods, DecoderThreadStarting() and
DecoderThreadStopped()
  * Allows us to establish a period when the decoder thread is not running and
it is safe to modify variables such as callbacks, that are only read when the
decoder thread is running.
  * Allows us to DCHECK thread guarantees.
  * Allows synchronizing callbacks from the module process thread and have them
only active while the decoder thread is running.
  * The above, allows us to establish two modes for the thread,
single-threaded-mutable and multi-threaded-const.
  * Using that knowledge, we can remove |receive_crit_| as well as locking for a
number of member variables.

MediaCodecVideoDecoder
* Removed frame polling code from this class, since this is now done from the
root thread function in VideoReceiveStream.

VideoReceiveStream
* On Android: Polls for decoded frames every 10ms (same interval as previously
in MediaCodecVideoDecoder)
* [Un]Registers the |video_receiver_| with the module thread only around the
time the decoder thread is started/stopped.
* Notifies the receiver of start/stop events of the decoder thread.
* Changed the decoder thread to use the new PlatformThread callback type.

BUG=webrtc:7361
==========

to

==========
Deliver video frames on Android, on the decode thread.

VideoCoding
* Adding a method for polling for frames on Android only until the capture
implementation takes care of this (longer term plan).

CodecDatabase
* Add an accessor for the current decoder
* Use std::unique_ptr<> for ownership.
* Remove "Release()" and "ReleaseDecoder()". Instead just delete.
* Remove |friend| relationship between CodecDatabase and VCMGenericDecoder.

VCMDecodedFrameCallback
* DCHECKs for thread correctness.
* Remove |lock_| now that a threading model has been established and verified.

VCMGenericDecoder
* All methods now have thread checks.
* Variable access associated with thread checkers.

VideoReceiver
* Added two notification methods, DecoderThreadStarting() and
DecoderThreadStopped()
  * Allows us to establish a period when the decoder thread is not running and
it is safe to modify variables such as callbacks, that are only read when the
decoder thread is running.
  * Allows us to DCHECK thread guarantees.
  * Allows synchronizing callbacks from the module process thread and have them
only active while the decoder thread is running.
  * The above, allows us to establish two modes for the thread,
single-threaded-mutable and multi-threaded-const.
  * Using that knowledge, we can remove |receive_crit_| as well as locking for a
number of member variables.

MediaCodecVideoDecoder
* Removed frame polling code from this class, since this is now done from the
root thread function in VideoReceiveStream.

VideoReceiveStream
* On Android: Polls for decoded frames every 10ms (same interval as previously
in MediaCodecVideoDecoder)
* [Un]Registers the |video_receiver_| with the module thread only around the
time the decoder thread is started/stopped.
* Notifies the receiver of start/stop events of the decoder thread.
* Changed the decoder thread to use the new PlatformThread callback type.

BUG=webrtc:7361, 695438
==========

[commit-bot: I haz the power, 2017-03-20 19:15:44]

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

[commit-bot: I haz the power, 2017-03-20 19:15:45]

Dry run: Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/23255)

[sakal, 2017-03-21 10:05:58]

I tried a call between Pixel and iPhone. I got constant output of:

03-21 10:51:57.136 28401 29372 W MediaCodecVideoDecoder: MediaCodecVideoDecoder:
Decoder is too far behind. Try to drain. Received: 207. Decoded: 202

I think this is because we need to feed multiple (up to 4) frames to the decoder
before it can produce output.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (left):

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:523: if (callback_ ==
NULL) {
Is this behavior change intended / relevant for the CL?

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:51: using rtc::Bind;
nit: indentation

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:589: ALOGD << "Decode() -
about to call DecodeOnCodecThread";
nit: Please remove excessive logging.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:594: ALOGD << "Decode() -
after call to DecodeOnCodecThread";
nit: Please remove excessive logging.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:830:
RTC_DCHECK_GE(slice_height, height);
If this check fails it result in reading outside of a buffer. Should we really
change it to a DCHECK?

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:952: ALOGD <<
"DeliverPendingOutputs: Have frame on the codec thread.";
This log seems a little excessive.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:962: ALOGD <<
"DeliverPendingOutputs: Decoded frame delivered.";
Same here.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:965: if (frames) {
I don't see frames ever actually being null. DCHECK instead?

[tommi, 2017-03-21 16:21:43]

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (left):

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:523: if (callback_ ==
NULL) {
On 2017/03/21 10:05:57, sakal wrote:
> Is this behavior change intended / relevant for the CL?

It's intended. The callback should never be null. It must be set before decoding
is started and only unset after the decoder thread has been stopped.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
File webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:51: using rtc::Bind;
On 2017/03/21 10:05:58, sakal wrote:
> nit: indentation

Done.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:589: ALOGD << "Decode() -
about to call DecodeOnCodecThread";
On 2017/03/21 10:05:57, sakal wrote:
> nit: Please remove excessive logging.

Done.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:594: ALOGD << "Decode() -
after call to DecodeOnCodecThread";
On 2017/03/21 10:05:58, sakal wrote:
> nit: Please remove excessive logging.

Done.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:830:
RTC_DCHECK_GE(slice_height, height);
On 2017/03/21 10:05:57, sakal wrote:
> If this check fails it result in reading outside of a buffer. Should we really
> change it to a DCHECK?

good point, changed back to a CHECK.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:952: ALOGD <<
"DeliverPendingOutputs: Have frame on the codec thread.";
On 2017/03/21 10:05:57, sakal wrote:
> This log seems a little excessive.

Done.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:962: ALOGD <<
"DeliverPendingOutputs: Decoded frame delivered.";
On 2017/03/21 10:05:58, sakal wrote:
> Same here.

Done.

https://codereview.webrtc.org/2764573002/diff/1/webrtc/sdk/android/src/jni/an...
webrtc/sdk/android/src/jni/androidmediadecoder_jni.cc:965: if (frames) {
On 2017/03/21 10:05:57, sakal wrote:
> I don't see frames ever actually being null. DCHECK instead?

Done.

[tommi, 2017-03-21 16:21:54]

Address comments

[tommi, 2017-03-21 16:23:21]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 16:23:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-21 17:07:47]

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

[commit-bot: I haz the power, 2017-03-21 17:07:48]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-03-22 09:02:37]

Wrt the "Decoder is too far behind" message.  Can you suggest what the right
thing to do is?

[sakal, 2017-03-22 13:23:01]

On 2017/03/22 09:02:37, tommi (webrtc) wrote:
> Wrt the "Decoder is too far behind" message.  Can you suggest what the right
> thing to do is?

I made a CL that seems to mostly fix the "Decoder is too far behind message":
https://codereview.webrtc.org/2767473005/

However, there is another problem:
03-22 13:50:34.973 12289 12381 W MediaCodecVideoDecoder: MediaCodecVideoDecoder:
Too many output buffers 2. Dropping frame with TS: 44900. Total number of
dropped frames: 356

I believe the source of the problem is that due to limitations in the texture
based frame passing mechanism, we can only have one frame in-flight at a time.
We have to render that frame first before we can deliver another output buffer.
With this CL, we try to store multiple frames into a vector, which is not
possible. Because the frames have to be delivered on the decoder thread, we
would have to pass control between these two threads multiple times, which is
not nice.

AFAIK, the decoder thread on Android exists for two reasons 1. allow polling 2.
handle method calls coming from different thread. Do you know which method calls
are still coming from other threads than the decoder thread? If we could move
these to the decoder thread or find other workarounds in the Android decoder, we
might be able to remove the codec thread.

[tommi, 2017-03-22 17:25:52]

On 2017/03/22 13:23:01, sakal wrote:
> On 2017/03/22 09:02:37, tommi (webrtc) wrote:
> > Wrt the "Decoder is too far behind" message.  Can you suggest what the right
> > thing to do is?
> 
> I made a CL that seems to mostly fix the "Decoder is too far behind message":
> https://codereview.webrtc.org/2767473005/
> 
> However, there is another problem:
> 03-22 13:50:34.973 12289 12381 W MediaCodecVideoDecoder:
MediaCodecVideoDecoder:
> Too many output buffers 2. Dropping frame with TS: 44900. Total number of
> dropped frames: 356
> 
> I believe the source of the problem is that due to limitations in the texture
> based frame passing mechanism, we can only have one frame in-flight at a time.
> We have to render that frame first before we can deliver another output
buffer.
> With this CL, we try to store multiple frames into a vector, which is not
> possible. Because the frames have to be delivered on the decoder thread, we
> would have to pass control between these two threads multiple times, which is
> not nice.
> 
> AFAIK, the decoder thread on Android exists for two reasons 1. allow polling
2.
> handle method calls coming from different thread. Do you know which method
calls
> are still coming from other threads than the decoder thread? If we could move
> these to the decoder thread or find other workarounds in the Android decoder,
we
> might be able to remove the codec thread.

Thanks for digging into this Sami.  Would it make sense to instead have a queue
in front of the decoder? (of undecoded frames)
It does seem like the interaction between the decoder on Android and
FrameBuffer, could be improved.

[sakal, 2017-03-23 08:26:36]

On 2017/03/22 17:25:52, tommi (webrtc) wrote:
> On 2017/03/22 13:23:01, sakal wrote:
> > On 2017/03/22 09:02:37, tommi (webrtc) wrote:
> > > Wrt the "Decoder is too far behind" message.  Can you suggest what the
right
> > > thing to do is?
> > 
> > I made a CL that seems to mostly fix the "Decoder is too far behind
message":
> > https://codereview.webrtc.org/2767473005/
> > 
> > However, there is another problem:
> > 03-22 13:50:34.973 12289 12381 W MediaCodecVideoDecoder:
> MediaCodecVideoDecoder:
> > Too many output buffers 2. Dropping frame with TS: 44900. Total number of
> > dropped frames: 356
> > 
> > I believe the source of the problem is that due to limitations in the
texture
> > based frame passing mechanism, we can only have one frame in-flight at a
time.
> > We have to render that frame first before we can deliver another output
> buffer.
> > With this CL, we try to store multiple frames into a vector, which is not
> > possible. Because the frames have to be delivered on the decoder thread, we
> > would have to pass control between these two threads multiple times, which
is
> > not nice.
> > 
> > AFAIK, the decoder thread on Android exists for two reasons 1. allow polling
> 2.
> > handle method calls coming from different thread. Do you know which method
> calls
> > are still coming from other threads than the decoder thread? If we could
move
> > these to the decoder thread or find other workarounds in the Android
decoder,
> we
> > might be able to remove the codec thread.
> 
> Thanks for digging into this Sami.  Would it make sense to instead have a
queue
> in front of the decoder? (of undecoded frames)
> It does seem like the interaction between the decoder on Android and
> FrameBuffer, could be improved.

I don't see how that would solve the problem. We are already buffering 1-4
frames on the input side and up to 3 frames on the output side inside the
decoder. The problem is we can only have one converted to webrtc::VideoFrame at
a time.

[tommi, 2017-03-28 08:50:37]

Rebase

[sakal, 2017-03-28 15:03:03]

Please integrate this CL https://codereview.webrtc.org/2767473005 and then we
can land this. I think the regression is small enough that we can just land
https://codereview.webrtc.org/2777203003/ right after this.

lgtm

[tommi, 2017-03-29 19:34:30]

incorporate fix from Sami

[tommi, 2017-03-29 19:35:09]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 19:35:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[tommi, 2017-03-29 19:35:40]

Thanks Sami - done.

Stefan, Magnus - can you take a look?

[commit-bot: I haz the power, 2017-03-29 19:57:44]

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

[commit-bot: I haz the power, 2017-03-29 19:57:45]

Dry run: This issue passed the CQ dry run.

[mflodman, 2017-03-31 09:30:01]

Only once comment, then LGTM.

Stefan,
Can you also take look on this from the JB - Decoder point of view?

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:176: if (ret <
WEBRTC_VIDEO_CODEC_OK) {
If it gets simpler, we can remove the return in this if and from here do
something like:

if (ret < WEBRTC_..._OK)
  Log ...

if (ret != WEBRTC_..._OK)
  Pop()

return ret;

[tommi, 2017-03-31 10:27:12]

Simplify error check

[tommi, 2017-03-31 10:27:19]

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:176: if (ret <
WEBRTC_VIDEO_CODEC_OK) {
On 2017/03/31 09:30:01, mflodman wrote:
> If it gets simpler, we can remove the return in this if and from here do
> something like:
> 
> if (ret < WEBRTC_..._OK)
>   Log ...
> 
> if (ret != WEBRTC_..._OK)
>   Pop()
> 
> return ret;

Done.

[stefan-webrtc, 2017-03-31 13:42:07]

lgtm

I wonder if we could do something to add a test case for the polling? Can we
test that it actually polls with the interval that we want it to, and thereby
avoid regressions if the jitter buffer fails to wake up after the timeout?

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_database.cc (left):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_database.cc:541: ReleaseDecoder(ptr_decoder);
Why don't we release here?

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:142: decoder_.release();
Nice improvement. :)

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:174: // time around?
Yes, but I think it can change on each call if it switches to a fallback
decoder?

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:517: // via a test only subclass?
If it is indeed only used by tests I think it's just that it hasn't been cleaned
up when the calling code was cleaned up.

[brandtr, 2017-03-31 14:49:35]

On 2017/03/31 13:42:07, stefan-webrtc wrote:
> lgtm
> 
> I wonder if we could do something to add a test case for the polling? Can we
> test that it actually polls with the interval that we want it to, and thereby
> avoid regressions if the jitter buffer fails to wake up after the timeout?
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/codec_database.cc (left):
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/codec_database.cc:541:
ReleaseDecoder(ptr_decoder);
> Why don't we release here?
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/generic_decoder.cc (right):
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/generic_decoder.cc:142: decoder_.release();
> Nice improvement. :)
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/generic_decoder.cc:174: // time around?
> Yes, but I think it can change on each call if it switches to a fallback
> decoder?
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> File webrtc/modules/video_coding/video_receiver.cc (right):
> 
>
https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
> webrtc/modules/video_coding/video_receiver.cc:517: // via a test only
subclass?
> If it is indeed only used by tests I think it's just that it hasn't been
cleaned
> up when the calling code was cleaned up.

I applied this patch to a test app, and it seems to lead to more dropped frames.
Will send you some logs.

[tommi, 2017-04-04 09:42:08]

Rebase

[tommi, 2017-04-04 10:13:39]

Update comments

[tommi, 2017-04-04 10:13:52]

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_database.cc (left):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_database.cc:541: ReleaseDecoder(ptr_decoder);
On 2017/03/31 13:42:07, stefan-webrtc wrote:
> Why don't we release here?

|ptr_decoder| is now a std::unique_ptr which goes out of scope here. The
|Release()| call has been removed and deleting the object does what Release()
did previously (and the two were always called together).

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:142: decoder_.release();
On 2017/03/31 13:42:07, stefan-webrtc wrote:
> Nice improvement. :)

:D

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.cc:174: // time around?
On 2017/03/31 13:42:07, stefan-webrtc wrote:
> Yes, but I think it can change on each call if it switches to a fallback
> decoder?

I'll leave it as is for now. Depending on the callback implementation, this
might not matter.

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2764573002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:517: // via a test only subclass?
On 2017/03/31 13:42:07, stefan-webrtc wrote:
> If it is indeed only used by tests I think it's just that it hasn't been
cleaned
> up when the calling code was cleaned up.

Ack. Updated TODO.

[tommi, 2017-04-04 10:15:46]

On 2017/03/31 13:42:07, stefan-webrtc wrote:
> I wonder if we could do something to add a test case for the polling? Can we
> test that it actually polls with the interval that we want it to, and thereby
> avoid regressions if the jitter buffer fails to wake up after the timeout?

Sami is working on a way to not need the polling at all. I'm hoping we can get
rid of it soon.

[tommi, 2017-04-04 10:18:38]

> I applied this patch to a test app, and it seems to lead to more dropped
frames.
> Will send you some logs.

Thanks for testing.  As we discussed offline, there were dropped frames in a
debug build but it didn't seem to be a big issue in release.  However I'd like
to keep an eye on it and ultimately I'm hopeful that Sami's improvements that
will get rid of the polling, will make it so that we won't have that problem.

[tommi, 2017-04-04 10:19:45]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2017-04-04 10:19:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org,
mflodman@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2764573002/#ps120001 (title:
"Update comments")

[commit-bot: I haz the power, 2017-04-04 10:19:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-04 10:53:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491301185002650,
"parent_rev": "aa7d935cd551b6911cacd459efd5be3b15e7ec6f", "commit_rev":
"e3aa88bbd5accadec73fa7e38584dfbf6aabe8a9"}

[commit-bot: I haz the power, 2017-04-04 10:53:06]

Description was changed from

==========
Deliver video frames on Android, on the decode thread.

VideoCoding
* Adding a method for polling for frames on Android only until the capture
implementation takes care of this (longer term plan).

CodecDatabase
* Add an accessor for the current decoder
* Use std::unique_ptr<> for ownership.
* Remove "Release()" and "ReleaseDecoder()". Instead just delete.
* Remove |friend| relationship between CodecDatabase and VCMGenericDecoder.

VCMDecodedFrameCallback
* DCHECKs for thread correctness.
* Remove |lock_| now that a threading model has been established and verified.

VCMGenericDecoder
* All methods now have thread checks.
* Variable access associated with thread checkers.

VideoReceiver
* Added two notification methods, DecoderThreadStarting() and
DecoderThreadStopped()
  * Allows us to establish a period when the decoder thread is not running and
it is safe to modify variables such as callbacks, that are only read when the
decoder thread is running.
  * Allows us to DCHECK thread guarantees.
  * Allows synchronizing callbacks from the module process thread and have them
only active while the decoder thread is running.
  * The above, allows us to establish two modes for the thread,
single-threaded-mutable and multi-threaded-const.
  * Using that knowledge, we can remove |receive_crit_| as well as locking for a
number of member variables.

MediaCodecVideoDecoder
* Removed frame polling code from this class, since this is now done from the
root thread function in VideoReceiveStream.

VideoReceiveStream
* On Android: Polls for decoded frames every 10ms (same interval as previously
in MediaCodecVideoDecoder)
* [Un]Registers the |video_receiver_| with the module thread only around the
time the decoder thread is started/stopped.
* Notifies the receiver of start/stop events of the decoder thread.
* Changed the decoder thread to use the new PlatformThread callback type.

BUG=webrtc:7361, 695438
==========

to

==========
Deliver video frames on Android, on the decode thread.

VideoCoding
* Adding a method for polling for frames on Android only until the capture
implementation takes care of this (longer term plan).

CodecDatabase
* Add an accessor for the current decoder
* Use std::unique_ptr<> for ownership.
* Remove "Release()" and "ReleaseDecoder()". Instead just delete.
* Remove |friend| relationship between CodecDatabase and VCMGenericDecoder.

VCMDecodedFrameCallback
* DCHECKs for thread correctness.
* Remove |lock_| now that a threading model has been established and verified.

VCMGenericDecoder
* All methods now have thread checks.
* Variable access associated with thread checkers.

VideoReceiver
* Added two notification methods, DecoderThreadStarting() and
DecoderThreadStopped()
  * Allows us to establish a period when the decoder thread is not running and
it is safe to modify variables such as callbacks, that are only read when the
decoder thread is running.
  * Allows us to DCHECK thread guarantees.
  * Allows synchronizing callbacks from the module process thread and have them
only active while the decoder thread is running.
  * The above, allows us to establish two modes for the thread,
single-threaded-mutable and multi-threaded-const.
  * Using that knowledge, we can remove |receive_crit_| as well as locking for a
number of member variables.

MediaCodecVideoDecoder
* Removed frame polling code from this class, since this is now done from the
root thread function in VideoReceiveStream.

VideoReceiveStream
* On Android: Polls for decoded frames every 10ms (same interval as previously
in MediaCodecVideoDecoder)
* [Un]Registers the |video_receiver_| with the module thread only around the
time the decoder thread is started/stopped.
* Notifies the receiver of start/stop events of the decoder thread.
* Changed the decoder thread to use the new PlatformThread callback type.

BUG=webrtc:7361, 695438

Review-Url: https://codereview.webrtc.org/2764573002
Cr-Commit-Position: refs/heads/master@{#17527}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e3aa88bbd5accadec73fa7e38...
==========

[commit-bot: I haz the power, 2017-04-04 10:53:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/e3aa88bbd5accadec73fa7e38...

[guidou, 2017-04-04 14:16:01]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.webrtc.org/2792033003/ by guidou@webrtc.org.

The reason for reverting is: Breaks Chrome FYI Android bots.

See:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%...
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%...
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%...
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%....