Fix perf and paint issues with VRI::PaintSingleFrame.

Fix perf and paint issues with VRI::PaintSingleFrame.

This prevents repeated rendering of the first frame and ensures the
algorithm knows about the frame being painted so that future calls
to OnLastFrameDropped() don't muck with statistics incorrectly.

Results seem slightly mixed, but maybe the official perf bot will
provide stronger evidence. It's a good change regardless.

BUG=627172, 656231
TEST=perf tests.

Committed: https://crrev.com/dee963030706f989b4fb7de973a916c02d2a2083
Cr-Commit-Position: refs/heads/master@{#427217}

[DaleCurtis, 2016-10-20 21:42:58]

Description was changed from

==========
Fix perf and paint issues with VRI::PaintSingleFrame.

BUG=
==========

to

==========
Fix perf and paint issues with VRI::PaintSingleFrame.

This prevents repeated rendering of the first frame and ensures the
algorithm knows about the frame being painted so that future calls
to OnLastFrameDropped() don't muck with statistics incorrectly.

Results seem slightly mixed, but maybe the official perf bot will
provide stronger evidence. It's a good change regardless.

BUG=627172, 656231
TEST=perf tests.
==========

[DaleCurtis, 2016-10-20 21:42:59]

dalecurtis@chromium.org changed reviewers:
+ chcunningham@chromium.org

[chcunningham, 2016-10-21 20:27:22]

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:79: if (!was_time_moving_ ||
!have_known_duration || render_interval_.is_zero()) {
Can you give me the context on the changes in this file? Its not clear to me why
we needed to expand this condition (and below) to account for
rendering_interval_ = 0. It seems like, if that is to occur on the first frame,
it should also be that we don't know the duration and/or (probably and) time is
not moving. I'm new to this class...

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.h (right):

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.h:114: // Returns the first frame in the
frame queue and
forgot to complete this guy

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.h:115: scoped_refptr<VideoFrame>
RenderFirstFrame();
Not seeing this in the .cc?

https://codereview.chromium.org/2437623004/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2437623004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:85: void VideoRendererImpl::Flush(const
base::Closure& callback) {
should you reset painted_first_frame_ here?

https://codereview.chromium.org/2437623004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:680: painted_first_frame_ = false;
to be sure I follow, the idea of resetting here is that, once the queue starts
to get even a single frame in it again, we'd like to go ahead and paint that
first frame (while still waiting for the queue to completely fill to resume
playback)?

[DaleCurtis, 2016-10-21 21:51:17]

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:79: if (!was_time_moving_ ||
!have_known_duration || render_interval_.is_zero()) {
On 2016/10/21 at 20:27:22, chcunningham wrote:
> Can you give me the context on the changes in this file? Its not clear to me
why we needed to expand this condition (and below) to account for
rendering_interval_ = 0. It seems like, if that is to occur on the first frame,
it should also be that we don't know the duration and/or (probably and) time is
not moving. I'm new to this class...

If we underflow we'll still know the duration. Hence why I deleted mention of
first frame :)

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.h (right):

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.h:114: // Returns the first frame in the
frame queue and
On 2016/10/21 at 20:27:22, chcunningham wrote:
> forgot to complete this guy

Deleted.

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.h:115: scoped_refptr<VideoFrame>
RenderFirstFrame();
On 2016/10/21 at 20:27:22, chcunningham wrote:
> Not seeing this in the .cc?

Deleted.

https://codereview.chromium.org/2437623004/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2437623004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:85: void VideoRendererImpl::Flush(const
base::Closure& callback) {
On 2016/10/21 at 20:27:22, chcunningham wrote:
> should you reset painted_first_frame_ here?

Yup, done.

https://codereview.chromium.org/2437623004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:680: painted_first_frame_ = false;
On 2016/10/21 at 20:27:22, chcunningham wrote:
> to be sure I follow, the idea of resetting here is that, once the queue starts
to get even a single frame in it again, we'd like to go ahead and paint that
first frame (while still waiting for the queue to completely fill to resume
playback)?

Correct.

[chcunningham, 2016-10-24 20:26:56]

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
media/filters/video_renderer_algorithm.cc:79: if (!was_time_moving_ ||
!have_known_duration || render_interval_.is_zero()) {
On 2016/10/21 21:51:17, DaleCurtis wrote:
> On 2016/10/21 at 20:27:22, chcunningham wrote:
> > Can you give me the context on the changes in this file? Its not clear to me
> why we needed to expand this condition (and below) to account for
> rendering_interval_ = 0. It seems like, if that is to occur on the first
frame,
> it should also be that we don't know the duration and/or (probably and) time
is
> not moving. I'm new to this class...
> 
> If we underflow we'll still know the duration. Hence why I deleted mention of
> first frame :)

Does render_interval_ = 0 when we underflow? IIUC, underflow means we don't have
any frames for the (typically non-zero) interval... I'm surprised to see it ever
be zero? Why does that happen?

[DaleCurtis, 2016-10-24 20:34:26]

On 2016/10/24 at 20:26:56, chcunningham wrote:
>
https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
> File media/filters/video_renderer_algorithm.cc (right):
> 
>
https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
> media/filters/video_renderer_algorithm.cc:79: if (!was_time_moving_ ||
!have_known_duration || render_interval_.is_zero()) {
> On 2016/10/21 21:51:17, DaleCurtis wrote:
> > On 2016/10/21 at 20:27:22, chcunningham wrote:
> > > Can you give me the context on the changes in this file? Its not clear to
me
> > why we needed to expand this condition (and below) to account for
> > rendering_interval_ = 0. It seems like, if that is to occur on the first
frame,
> > it should also be that we don't know the duration and/or (probably and) time
is
> > not moving. I'm new to this class...
> > 
> > If we underflow we'll still know the duration. Hence why I deleted mention
of
> > first frame :)
> 
> Does render_interval_ = 0 when we underflow? IIUC, underflow means we don't
have any frames for the (typically non-zero) interval... I'm surprised to see it
ever be zero? Why does that happen?

It equals zero when we pass in two null timeticks in this change :)

[chcunningham, 2016-10-24 20:38:50]

On 2016/10/24 20:34:26, DaleCurtis wrote:
> On 2016/10/24 at 20:26:56, chcunningham wrote:
> >
>
https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
> > File media/filters/video_renderer_algorithm.cc (right):
> > 
> >
>
https://codereview.chromium.org/2437623004/diff/1/media/filters/video_rendere...
> > media/filters/video_renderer_algorithm.cc:79: if (!was_time_moving_ ||
> !have_known_duration || render_interval_.is_zero()) {
> > On 2016/10/21 21:51:17, DaleCurtis wrote:
> > > On 2016/10/21 at 20:27:22, chcunningham wrote:
> > > > Can you give me the context on the changes in this file? Its not clear
to
> me
> > > why we needed to expand this condition (and below) to account for
> > > rendering_interval_ = 0. It seems like, if that is to occur on the first
> frame,
> > > it should also be that we don't know the duration and/or (probably and)
time
> is
> > > not moving. I'm new to this class...
> > > 
> > > If we underflow we'll still know the duration. Hence why I deleted mention
> of
> > > first frame :)
> > 
> > Does render_interval_ = 0 when we underflow? IIUC, underflow means we don't
> have any frames for the (typically non-zero) interval... I'm surprised to see
it
> ever be zero? Why does that happen?
> 
> It equals zero when we pass in two null timeticks in this change :)

Ah, got it!

[chcunningham, 2016-10-24 20:38:55]

lgtm

[DaleCurtis, 2016-10-24 23:53:30]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-10-24 23:54:03]

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

[commit-bot: I haz the power, 2016-10-25 01:31:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-25 01:34:07]

Description was changed from

==========
Fix perf and paint issues with VRI::PaintSingleFrame.

This prevents repeated rendering of the first frame and ensures the
algorithm knows about the frame being painted so that future calls
to OnLastFrameDropped() don't muck with statistics incorrectly.

Results seem slightly mixed, but maybe the official perf bot will
provide stronger evidence. It's a good change regardless.

BUG=627172, 656231
TEST=perf tests.
==========

to

==========
Fix perf and paint issues with VRI::PaintSingleFrame.

This prevents repeated rendering of the first frame and ensures the
algorithm knows about the frame being painted so that future calls
to OnLastFrameDropped() don't muck with statistics incorrectly.

Results seem slightly mixed, but maybe the official perf bot will
provide stronger evidence. It's a good change regardless.

BUG=627172, 656231
TEST=perf tests.

Committed: https://crrev.com/dee963030706f989b4fb7de973a916c02d2a2083
Cr-Commit-Position: refs/heads/master@{#427217}
==========

[commit-bot: I haz the power, 2016-10-25 01:34:08]

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