Avoid unnecessary post task during frame delivery.

Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indices. This
speeds up frame metadata delivery to the renderer.

In total this reduces the total time from NotifyPictureReady() to
FrameReady() from ~[2.5ms, 9.8ms] to around ~[0.7ms,1.5ms].

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.

Committed: https://crrev.com/427a97695071d8b7662dd668cb844477dfb707bf
Cr-Commit-Position: refs/heads/master@{#389671}

[DaleCurtis, 2016-04-25 19:04:16]

dalecurtis@chromium.org changed reviewers:
+ liberato@chromium.org, xhwang@chromium.org

[DaleCurtis, 2016-04-25 19:06:47]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 19:07:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921803002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921803002/1

[DaleCurtis, 2016-04-25 19:07:33]

Description was changed from

==========
Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indicies. This
speeds up frame metadata delivery to the renderer.

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.
==========

to

==========
Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indices. This
speeds up frame metadata delivery to the renderer.

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.
==========

[liberato (no reviews please), 2016-04-25 19:58:22]

lgtm

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:841:
NotifyPictureReady(picture);
how much time does this buy?

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:474: {
in case Assign => DoIOTask => PictureReady?  deja vu.  :)

[xhwang, 2016-04-25 21:15:55]

lg with one comment

https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h#...
media/base/video_decoder.h:70: // delivery times with high-frame-rate material.
We typically expect callbacks (including the output_cb) to be fired
asynchronously, to avoid reentrancy issues, e.g.

Decode() -> DecodeBuffer() -> output_cb.Run()

Looking at DecoderStream, this may be okay. But it will definitely make our
callstack more complicated. So I wonder how much performance gain does this buy
us? Could you please make it more clear, here and in DecoderStream, that this
means output_cb.Run() could be fired in the same call stack as Decode()?

[DaleCurtis, 2016-04-25 21:31:44]

Since there were some performance questions, let me be clear, w/o these changes
(specifically #1), we underflow constantly while playing 720p60 content :)

https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h#...
media/base/video_decoder.h:70: // delivery times with high-frame-rate material.
On 2016/04/25 at 21:15:55, xhwang wrote:
> We typically expect callbacks (including the output_cb) to be fired
asynchronously, to avoid reentrancy issues, e.g.
> 
> Decode() -> DecodeBuffer() -> output_cb.Run()
> 
> Looking at DecoderStream, this may be okay. But it will definitely make our
callstack more complicated. So I wonder how much performance gain does this buy
us? Could you please make it more clear, here and in DecoderStream, that this
means output_cb.Run() could be fired in the same call stack as Decode()?

This definitely buys us a lot of performance gain in the case of
GpuVideoDecoder, i.e. w/o this fix we're unable to play 720p60 content without
constantly underflowing.

[xhwang, 2016-04-25 21:38:02]

On 2016/04/25 21:31:44, DaleCurtis wrote:
> Since there were some performance questions, let me be clear, w/o these
changes
> (specifically #1), we underflow constantly while playing 720p60 content :)
> 
> https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h
> File media/base/video_decoder.h (right):
> 
>
https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h#...
> media/base/video_decoder.h:70: // delivery times with high-frame-rate
material.
> On 2016/04/25 at 21:15:55, xhwang wrote:
> > We typically expect callbacks (including the output_cb) to be fired
> asynchronously, to avoid reentrancy issues, e.g.
> > 
> > Decode() -> DecodeBuffer() -> output_cb.Run()
> > 
> > Looking at DecoderStream, this may be okay. But it will definitely make our
> callstack more complicated. So I wonder how much performance gain does this
buy
> us? Could you please make it more clear, here and in DecoderStream, that this
> means output_cb.Run() could be fired in the same call stack as Decode()?
> 
> This definitely buys us a lot of performance gain in the case of
> GpuVideoDecoder, i.e. w/o this fix we're unable to play 720p60 content without
> constantly underflowing.

Thanks for the info! GVD/VDA is actually different. Given the nature of IPC, we
won't have reentrancy issues (at least into our media pipeline). Also, in that
case, we are playing with the VDA inteface, not the VideoDecoder interface.

For the VideoDecoder interface change, can we at least have some comments about
the potential reentrancy issue?

[DaleCurtis, 2016-04-25 22:29:41]

Description was changed from

==========
Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indices. This
speeds up frame metadata delivery to the renderer.

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.
==========

to

==========
Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indices. This
speeds up frame metadata delivery to the renderer.

In total this reduces the total time from NotifyPictureReady() to
FrameReady() from ~[2.5ms, 9.8ms] to around ~[0.7ms,1.5ms].

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.
==========

[DaleCurtis, 2016-04-25 22:32:22]

Updated comments and timing information. You can find the timing logs at

http://storage.googleapis.com/dalecurtis/pre-fix.log
http://storage.googleapis.com/dalecurtis/post-fix.log

If you want to mine the raw data.

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:841:
NotifyPictureReady(picture);
On 2016/04/25 at 19:58:22, liberato wrote:
> how much time does this buy?

You mean not posting or the reorder? Combined this saves ~1.5ms to ~3.5ms
depending on if we release to the back buffer or not.

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1921803002/diff/1/content/common/gpu/media/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:474: {
On 2016/04/25 at 19:58:22, liberato wrote:
> in case Assign => DoIOTask => PictureReady?  deja vu.  :)

Yup, exactly.

https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/1921803002/diff/1/media/base/video_decoder.h#...
media/base/video_decoder.h:70: // delivery times with high-frame-rate material.
On 2016/04/25 at 21:31:44, DaleCurtis wrote:
> On 2016/04/25 at 21:15:55, xhwang wrote:
> > We typically expect callbacks (including the output_cb) to be fired
asynchronously, to avoid reentrancy issues, e.g.
> > 
> > Decode() -> DecodeBuffer() -> output_cb.Run()
> > 
> > Looking at DecoderStream, this may be okay. But it will definitely make our
callstack more complicated. So I wonder how much performance gain does this buy
us? Could you please make it more clear, here and in DecoderStream, that this
means output_cb.Run() could be fired in the same call stack as Decode()?
> 
> This definitely buys us a lot of performance gain in the case of
GpuVideoDecoder, i.e. w/o this fix we're unable to play 720p60 content without
constantly underflowing.

Done. Fixed the existing comment too which wasn't true.

[xhwang, 2016-04-25 22:40:28]

LGTM % one last comment

https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decode...
media/base/video_decoder.h:94: // trampoline as necessary.
Note that we requires that the |decode_cb| must NOT be called from within this
method. If the caller trampolines/posts the output_cb and still want to make
sure output_cb is called before decode_cb, then it needs to post the decode_cb
again. This makes me a bit sad. Maybe we should just say something like "the
calls must handles this case correctly".

[DaleCurtis, 2016-04-25 22:52:16]

https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decode...
media/base/video_decoder.h:94: // trampoline as necessary.
On 2016/04/25 at 22:40:28, xhwang wrote:
> Note that we requires that the |decode_cb| must NOT be called from within this
method. If the caller trampolines/posts the output_cb and still want to make
sure output_cb is called before decode_cb, then it needs to post the decode_cb
again. This makes me a bit sad. Maybe we should just say something like "the
calls must handles this case correctly".

Do we need to say anything? If |output_cb| runs while Decode() is in flight, it
will execute before |decode_cb| is posted. I don't think anything else is
necessary but a BindToCurrentLoop.

[xhwang, 2016-04-25 22:58:42]

On 2016/04/25 22:52:16, DaleCurtis wrote:
>
https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decoder.h
> File media/base/video_decoder.h (right):
> 
>
https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decode...
> media/base/video_decoder.h:94: // trampoline as necessary.
> On 2016/04/25 at 22:40:28, xhwang wrote:
> > Note that we requires that the |decode_cb| must NOT be called from within
this
> method. If the caller trampolines/posts the output_cb and still want to make
> sure output_cb is called before decode_cb, then it needs to post the decode_cb
> again. This makes me a bit sad. Maybe we should just say something like "the
> calls must handles this case correctly".
> 
> Do we need to say anything? If |output_cb| runs while Decode() is in flight,
it
> will execute before |decode_cb| is posted. I don't think anything else is
> necessary but a BindToCurrentLoop.

Strictly speaking, no. But it could be tricky. For example, if the caller
blindly use BTCL for |output_cb|. Then the posted |output_cb| could arrive after
|decode_cb|. It doesn't violates any API but could be a bit confusing. I was
just talking about adding some comment that could help make this more clear.

[DaleCurtis, 2016-04-25 23:07:43]

Thanks for review!

https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/1921803002/diff/20001/media/base/video_decode...
media/base/video_decoder.h:94: // trampoline as necessary.
On 2016/04/25 at 22:52:16, DaleCurtis wrote:
> On 2016/04/25 at 22:40:28, xhwang wrote:
> > Note that we requires that the |decode_cb| must NOT be called from within
this method. If the caller trampolines/posts the output_cb and still want to
make sure output_cb is called before decode_cb, then it needs to post the
decode_cb again. This makes me a bit sad. Maybe we should just say something
like "the calls must handles this case correctly".
> 
> Do we need to say anything? If |output_cb| runs while Decode() is in flight,
it will execute before |decode_cb| is posted. I don't think anything else is
necessary but a BindToCurrentLoop.

Discussed offline, while the existing behavior is preserved, the use of "may" in
the comment would cause issues when blindly using BTCL -- so that comment has
been removed in favor of the simpler: "callers must handle this case." Which is
already the case everywhere.

[DaleCurtis, 2016-04-25 23:07:45]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-04-25 23:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1921803002/#ps20001
(title: "Comments.")

[commit-bot: I haz the power, 2016-04-25 23:08:21]

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

[commit-bot: I haz the power, 2016-04-26 02:31:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 02:31:58]

Description was changed from

==========
Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indices. This
speeds up frame metadata delivery to the renderer.

In total this reduces the total time from NotifyPictureReady() to
FrameReady() from ~[2.5ms, 9.8ms] to around ~[0.7ms,1.5ms].

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.
==========

to

==========
Avoid unnecessary post task during frame delivery.

With the three changes in this CL, primarily the first, we can now
play 720p60 and 1080p60 content on the Moto X and N7v2 devices when
combined with the underflow fixes from http://crrev.com/1863373002.

There are three places we were able to avoid unnecessary thread hops,
ordered in terms of impact:
- VD::|output_cb| does not need to PostTask since it's already
delivered many calls after provided. This change had the most impact,
which suggests that the media thread was blocked with other tasks
while the frame is delivered. All VideoDecoders are updated.
- Don't trampoline GVD::ReleaseMailbox unless necessary, this also
runs on the media thread when frames are dropped before being sent
to the compositor. This allows underflow to catch up more quickly.
- Don't trampoline for NotifyPictureRead() in AVDA and notify before
attempting to release to backbuffer and setting codec indices. This
speeds up frame metadata delivery to the renderer.

In total this reduces the total time from NotifyPictureReady() to
FrameReady() from ~[2.5ms, 9.8ms] to around ~[0.7ms,1.5ms].

BUG=601066
TEST=720p60 and 1080p60 playback are smooth on MotoX and N7v2.

Committed: https://crrev.com/427a97695071d8b7662dd668cb844477dfb707bf
Cr-Commit-Position: refs/heads/master@{#389671}
==========

[commit-bot: I haz the power, 2016-04-26 02:31:59]

Patchset 2 (id:??) landed as
https://crrev.com/427a97695071d8b7662dd668cb844477dfb707bf
Cr-Commit-Position: refs/heads/master@{#389671}