Remove requirement of compositing a video frame for dropped frame count.

Remove requirement of compositing a video frame for dropped frame count.

It's impossible for VideoFrameCompositor to know why the compositor may not have requested the frame, so it shouldn't be counted towards dropped frames if the rest of the media stack was capable of producing and delivering frames on time.

BUG=335345
R=xhwang@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260358

[scherkus (not reviewing), 2014-03-27 20:58:22]

xhwang: can you take a look?

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.cc (left):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:69: current_frame_composited_ =
false;
this was a bug ... but unittests didn't catch it because in reality there isn't
a situation where we should be increasing the dropped frame count because our
frame wasn't composited

[dshwang, 2014-03-27 21:17:00]

Hi, I'm coming here from crrev.com/212623009

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.cc (left):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:69: current_frame_composited_ =
false;
On 2014/03/27 20:58:22, scherkus wrote:
> this was a bug ... but unittests didn't catch it because in reality there
isn't
> a situation where we should be increasing the dropped frame count because our
> frame wasn't composited

I couldn't understand.
It is possible that VideoLayer received DidReceiveFrame() callback but doesn't
call GetCurrentFrame(true) yet.
In that case, we should increase frames_dropped_before_composite_ and then drop
current frame.
Could you let me know?, if I'm missing something.

[xhwang, 2014-03-27 21:28:59]

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.cc (right):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:45: }
Merge this to l.54-55? e.g.

if (!compositor_notify_finished_) {
  if (frames_dropped_before_composite_ < kuint32max)
    ++frames_dropped_before_composite_;
  return;
}

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:54: if
(!compositor_notify_finished_)
Since we check the negative, how about renaming it to something like
compositor_notify_pending?

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:55: return;
OOC, what's the chance for this to happen? It seems the "notification" only
involves a post task, which in theory should be pretty fast.

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:75: virtual void
SetVideoFrameProviderClient(
Now we are making this public, anything changed?

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.h (right):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.h:63: uint32
GetFramesDroppedBeforeComposite();
The name is not accurate now. "BeforeComposite" should really be
"BeforeCompositorIsNotified". Previously a frame is counted as dropped if it's
updated before the frame is composited. Now we don't care whether it's
composited or not.

That being said, I don't have a good naming suggestion yet... Probably just drop
the "BeforeComposite" part?

[scherkus (not reviewing), 2014-03-29 00:34:14]

PTAL

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.cc (left):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:69: current_frame_composited_ =
false;
On 2014/03/27 21:17:01, dshwang wrote:
> On 2014/03/27 20:58:22, scherkus wrote:
> > this was a bug ... but unittests didn't catch it because in reality there
> isn't
> > a situation where we should be increasing the dropped frame count because
our
> > frame wasn't composited
> 
> I couldn't understand.
> It is possible that VideoLayer received DidReceiveFrame() callback but doesn't
> call GetCurrentFrame(true) yet.
> In that case, we should increase frames_dropped_before_composite_ and then
drop
> current frame.
> Could you let me know?, if I'm missing something.

the original intent was to increment dropped frames if we GCF(true) wasn't
called in time ... but this doesn't work in practice since we can't tell why
GCF(true) wasn't called (e.g., compositor was late vs. compositor knows it
doesn't need to composite video for the current frame)

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.cc (right):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:45: }
On 2014/03/27 21:28:59, xhwang wrote:
> Merge this to l.54-55? e.g.
> 
> if (!compositor_notify_finished_) {
>   if (frames_dropped_before_composite_ < kuint32max)
>     ++frames_dropped_before_composite_;
>   return;
> }

Done.

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:54: if
(!compositor_notify_finished_)
On 2014/03/27 21:28:59, xhwang wrote:
> Since we check the negative, how about renaming it to something like
> compositor_notify_pending?

Done.

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:55: return;
On 2014/03/27 21:28:59, xhwang wrote:
> OOC, what's the chance for this to happen? It seems the "notification" only
> involves a post task, which in theory should be pretty fast.

to be honest I'm not 100% sure

I've definitely logged traces where the threaded compositor can get quite bogged
down on complex pages on underpowered hardware

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.cc:75: virtual void
SetVideoFrameProviderClient(
On 2014/03/27 21:28:59, xhwang wrote:
> Now we are making this public, anything changed?

nope -- it was mostly motivated by being able to call a single GCF() instead of
carrying around two

considering this is an Internal class I feel it's OK

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
File content/renderer/media/video_frame_compositor.h (right):

https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
content/renderer/media/video_frame_compositor.h:63: uint32
GetFramesDroppedBeforeComposite();
On 2014/03/27 21:28:59, xhwang wrote:
> The name is not accurate now. "BeforeComposite" should really be
> "BeforeCompositorIsNotified". Previously a frame is counted as dropped if it's
> updated before the frame is composited. Now we don't care whether it's
> composited or not.
> 
> That being said, I don't have a good naming suggestion yet... Probably just
drop
> the "BeforeComposite" part?

let's go with the extremely long but accurate name

I feel it's OK for something like this where it's extremely important to
understand exactly what we're measuring

[xhwang, 2014-03-29 01:02:27]

lgtm

[scherkus (not reviewing), 2014-03-29 01:47:36]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-03-29 01:49:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/214823005/60001

[commit-bot: I haz the power, 2014-03-29 03:18:18]

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

[commit-bot: I haz the power, 2014-03-29 03:18:18]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[dshwang, 2014-03-29 04:16:59]

On 2014/03/29 00:34:14, scherkus wrote:
> PTAL
> 
>
https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
> File content/renderer/media/video_frame_compositor.cc (left):
> 
>
https://codereview.chromium.org/214823005/diff/1/content/renderer/media/video...
> content/renderer/media/video_frame_compositor.cc:69: current_frame_composited_
=
> false;
> On 2014/03/27 21:17:01, dshwang wrote:
> > On 2014/03/27 20:58:22, scherkus wrote:
> > > this was a bug ... but unittests didn't catch it because in reality there
> > isn't
> > > a situation where we should be increasing the dropped frame count because
> our
> > > frame wasn't composited
> > 
> > I couldn't understand.
> > It is possible that VideoLayer received DidReceiveFrame() callback but
doesn't
> > call GetCurrentFrame(true) yet.
> > In that case, we should increase frames_dropped_before_composite_ and then
> drop
> > current frame.
> > Could you let me know?, if I'm missing something.
> 
> the original intent was to increment dropped frames if we GCF(true) wasn't
> called in time ... but this doesn't work in practice since we can't tell why
> GCF(true) wasn't called (e.g., compositor was late vs. compositor knows it
> doesn't need to composite video for the current frame)

Thank you for explanation. The reason makes sense. I guess you'll resolve the
case which I mentioned above.

[scherkus (not reviewing), 2014-03-29 04:27:02]

Committed patchset #3 manually as r260358 (presubmit successful).