Combine bytes-decoded notification with memory usage and frame count.

Combine bytes-decoded notification with memory usage and frame count.

This patch attempts to reduce the number of notifications being posted
to the main thread and causing performance issues. To further reduce
the number of notification we could consider combining AudioRendererImpl
and VideoRendererImpl notifications in RendererImpl before sending it
to PipelineImpl.

BUG=619975

[alokp, 2016-06-17 22:43:22]

alokp@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sandersd@chromium.org

[alokp, 2016-06-17 22:43:23]

Perf tryjob is here: https://codereview.chromium.org/2075293003/

[alokp, 2016-06-17 22:45:53]

Description was changed from

==========
Combine bytes-decoded notification with memory usage and frame count.

BUG=619975
==========

to

==========
Combine bytes-decoded notification with memory usage and frame count.

This patch attempts to reduce the number of notifications being posted
to the main thread and causing performance issues. To further reduce
the number of notification we could consider combining AudioRendererImpl
and VideoRendererImpl notifications in RendererImpl before sending it
to PipelineImpl.

BUG=619975
==========

[DaleCurtis, 2016-06-17 23:03:17]

https://codereview.chromium.org/2079923002/diff/20001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2079923002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:162:
base::Bind(&VideoRendererImpl::BytesDecoded, weak_factory_.GetWeakPtr()),
This is only returned when a frame is decoded, so I wonder if we should return
it instead as metadata on each video frame and drop this entire callback.

[alokp, 2016-06-18 04:18:06]

https://codereview.chromium.org/2079923002/diff/20001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2079923002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:162:
base::Bind(&VideoRendererImpl::BytesDecoded, weak_factory_.GetWeakPtr()),
On 2016/06/17 23:03:17, DaleCurtis wrote:
> This is only returned when a frame is decoded, so I wonder if we should return
> it instead as metadata on each video frame and drop this entire callback.

We could do that, but what do you want to do for AudioRendererImpl? We need to
do something symmetric for both Audio and Video renderers because they both use
DecoderStream<DemuxerStream::Type>.

FWIW this callback is pretty cheap now.

[alokp, 2016-06-18 04:19:25]

New tryrun because I forgot to specify pageset-repeat in the last run:
https://codereview.chromium.org/2080483002/

[DaleCurtis, 2016-06-20 18:45:39]

Results:
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
-- I forget what the original values were but it still looks like there's some
issues (or that this trades some issues for others).

We could also make this information part of the DecodeCB for Audio/Video if we
don't want to add it as metadata. I agree we'd need a new "AudioBufferMetadata"
type class otherwise.

[alokp, 2016-06-21 22:12:21]

On 2016/06/20 18:45:39, DaleCurtis wrote:
> Results:
>
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> -- I forget what the original values were but it still looks like there's some
> issues (or that this trades some issues for others).

The patch did reduce the number of statistics notifications by half during
playback which may be why numbers for most test pages improved or remained the
same. I am not sure why the numbers for others regressed though. Anyways I think
the main culprit here is OnBufferedTimeRangesChange notifications. And I do not
know how to define a sensible throttle rate for them. IMO
https://chromiumcodereview.appspot.com/2071503003/ is a better fix. It
accumulates all notifications in media thread. Main thread can grab the stats
whenever or at whatever frequency.

> We could also make this information part of the DecodeCB for Audio/Video if we
> don't want to add it as metadata. I agree we'd need a new
"AudioBufferMetadata"
> type class otherwise.

Adding it to DecodeCB sgtm. Although I realized that this patch disables the
bytes-decoded notification when playback rate == 0, but decoder is still active.
Is that OK?

[DaleCurtis, 2016-06-21 23:50:03]

On 2016/06/21 at 22:12:21, alokp wrote:
> On 2016/06/20 18:45:39, DaleCurtis wrote:
> > Results:
> >
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06...
> > -- I forget what the original values were but it still looks like there's
some
> > issues (or that this trades some issues for others).
> 
> The patch did reduce the number of statistics notifications by half during
playback which may be why numbers for most test pages improved or remained the
same. I am not sure why the numbers for others regressed though. Anyways I think
the main culprit here is OnBufferedTimeRangesChange notifications. And I do not
know how to define a sensible throttle rate for them. IMO
https://chromiumcodereview.appspot.com/2071503003/ is a better fix. It
accumulates all notifications in media thread. Main thread can grab the stats
whenever or at whatever frequency.

Can we remove the posting entirely and just update local variables that can be
queried under lock?

> 
> > We could also make this information part of the DecodeCB for Audio/Video if
we
> > don't want to add it as metadata. I agree we'd need a new
"AudioBufferMetadata"
> > type class otherwise.
> 
> Adding it to DecodeCB sgtm. Although I realized that this patch disables the
bytes-decoded notification when playback rate == 0, but decoder is still active.
Is that OK?

I think that's fine.

[alokp, 2016-06-22 02:35:04]

> Can we remove the posting entirely and just update local variables that can be
> queried under lock?

Yes - the proposed patch (https://codereview.chromium.org/2071503003) removes
posting of OnBufferedTimeRangesChanged entirely. I also verified that both
Demuxer implementations - Chunk and FFmpeg - call this function on DemuxerHost
directly.

I will use this patch to move bytes-decoded notification into DecodeCB and
eliminate posting of OnStatisticsUpdate.

Can you review the other patch (https://codereview.chromium.org/2071503003)
which is really a partial revert of https://codereview.chromium.org/1999893004
and fixes the performance regression. Thanks!

[DaleCurtis, 2016-06-22 15:12:34]

Yeah, I think that sounds good and is more efficient.