vda_unittest - Move the fps control from ThrottleVDAClient to RenderingHelper.

vda_unittest - Move the fps control from ThrottleVDAClient to RenderingHelper.

RenderingHelper is now responsible for controlling the rendering fps. There is no need for the ThrottleVDAClient to do the same thing.

BUG=None
TEST=Run the program on peach-pit

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

[Owen Lin, 2014-05-21 05:47:10]

PTAL. Thanks.

This is the second part of the change to the rendering helper.

I tested it with a 1080p video, the fps is improved from 30 fps to 60 fps.
However, the fps is limited by the monitor refresh rate. Because the SwapBuffer
become blocking after using the fullscreen window. I tried set the SwapInterval
to 0 but no help.

I don't think this is a big issue since it is a hardware limitation, we cannot
really show more than that frames on the screen. 

There is one thing I didn't understand. When I tried to raise the fps over 60,
the actual fps is dropped (to 40 fps).

[Ami GONE FROM CHROMIUM, 2014-05-21 18:25:54]

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
File content/common/gpu/media/rendering_helper.h (right):

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:54: class RenderingClient {
what is this?
(please document all classes, _especially_ interface classes).

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:56: virtual void
RenderContent(RenderingHelper* helper) = 0;
Prefer callbacks to interface types like this.  E.g.
typedef base::Callback<void(RenderingHelper*)> RenderContentCB;

probably belongs inside RenderingHelper.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:61: // Creates and draws textures
used by the video decoder.
"draws" is now a lie?
Comment needs updating.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:76: void SetClient(int window_id,
RenderingClient* client);
Would be easier to grok as a param to a once-only call; Initialize or
CreateTexture.
Bind to a weakptr that gets invalidated instead of 
SetClient(i, NULL);

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:246: virtual void
RenderContent(RenderingHelper*) OVERRIDE;
Doesn't belong to the comment at l.234.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:473: // Frame
dropped, just redraw the last texture.
why is this a good idea?
(vs. just doing nothing)

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:475:
picture_buffer->texture_id());
indent is off
(here and elsewhere; please clang-format)

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:485: if
(pending_picture_buffer_ids_.size() <= 1) {
can't be <1 so replace with ==1

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:487: } else {
don't need to pending_picture_updated_=true; ?

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:522: if
(render_as_thumbnails_) {
It's unfortunate that the first and last cases are handled completely here but
the middle case sets up pending state for RenderContent to consume later.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:546: if
(decoder_deleted())
I think this sort of check could be dropped if you went the weakptr/Bind route
for RenderContent.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:591:
pending_picture_buffer_ids_.pop_front();
pending_picture_updated_ = false;
?

[Owen Lin, 2014-05-26 06:21:07]

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
File content/common/gpu/media/rendering_helper.h (right):

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:54: class RenderingClient {
On 2014/05/21 18:25:54, Ami Fischman wrote:
> what is this?
> (please document all classes, _especially_ interface classes).

Done.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:56: virtual void
RenderContent(RenderingHelper* helper) = 0;
On 2014/05/21 18:25:54, Ami Fischman wrote:
> Prefer callbacks to interface types like this.  E.g.
> typedef base::Callback<void(RenderingHelper*)> RenderContentCB;
> 
> probably belongs inside RenderingHelper.
Moved into RenderingHelper.
I am going to add some other methods to this interface. So, keep it as interface
instead of callbacks.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:61: // Creates and draws textures
used by the video decoder.
On 2014/05/21 18:25:54, Ami Fischman wrote:
> "draws" is now a lie?
> Comment needs updating.
This class is the one who really draws textures.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:76: void SetClient(int window_id,
RenderingClient* client);
On 2014/05/21 18:25:54, Ami Fischman wrote:
> Would be easier to grok as a param to a once-only call; Initialize or
> CreateTexture.
> Bind to a weakptr that gets invalidated instead of 
> SetClient(i, NULL);
Passed as the parameter of Initialize().

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:246: virtual void
RenderContent(RenderingHelper*) OVERRIDE;
On 2014/05/21 18:25:54, Ami Fischman wrote:
> Doesn't belong to the comment at l.234.

Done.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:473: // Frame
dropped, just redraw the last texture.
On 2014/05/21 18:25:54, Ami Fischman wrote:
> why is this a good idea?
> (vs. just doing nothing)
When doing nothing, the video flickers. It swaps between the previous two
frames.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:475:
picture_buffer->texture_id());
On 2014/05/21 18:25:54, Ami Fischman wrote:
> indent is off
> (here and elsewhere; please clang-format)

Done.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:485: if
(pending_picture_buffer_ids_.size() <= 1) {
On 2014/05/21 18:25:54, Ami Fischman wrote:
> can't be <1 so replace with ==1

Done.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:487: } else {
On 2014/05/21 18:25:54, Ami Fischman wrote:
> don't need to pending_picture_updated_=true; ?

We get here only when pending_picture_updated == true.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:546: if
(decoder_deleted())
On 2014/05/21 18:25:54, Ami Fischman wrote:
> I think this sort of check could be dropped if you went the weakptr/Bind route
> for RenderContent.

For the TearDownTiming tests, we could delete the decoder at any states. In my
test, it crashed if it get deleted while RESETTING.

https://codereview.chromium.org/294663006/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:591:
pending_picture_buffer_ids_.pop_front();
On 2014/05/21 18:25:54, Ami Fischman wrote:
> pending_picture_updated_ = false;
> ?
Thanks. Done.
However, the initial value should be true. (for the first frame).

[Ami GONE FROM CHROMIUM, 2014-05-28 16:09:25]

LGTM

https://codereview.chromium.org/294663006/diff/80001/content/common/gpu/media...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/294663006/diff/80001/content/common/gpu/media...
content/common/gpu/media/video_decode_accelerator_unittest.cc:211: //
|suppress_rendering| indicates GL rendering is supressed or not.
s/supressed/suppressed/

[Owen Lin, 2014-06-11 06:12:38]

The CQ bit was checked by owenlin@chromium.org

[commit-bot: I haz the power, 2014-06-11 06:18:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/owenlin@chromium.org/294663006/100001

[Owen Lin, 2014-06-11 06:44:42]

Hi, 

Ami has leaved the team and no longer the owner. Can you lgtm this change?
Thanks.

[commit-bot: I haz the power, 2014-06-11 12:21:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-11 12:25:45]

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

[commit-bot: I haz the power, 2014-06-11 12:25:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[scherkus (not reviewing), 2014-06-11 16:02:14]

lgtm

[Owen Lin, 2014-06-12 02:02:53]

The CQ bit was checked by owenlin@chromium.org

[commit-bot: I haz the power, 2014-06-12 02:04:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/owenlin@chromium.org/294663006/100001

[commit-bot: I haz the power, 2014-06-12 03:42:02]

Change committed as 276519