OmxVideoDecodeAcceleratorTest is born!

OmxVideoDecodeAcceleratorTest is born!

This exercises OmxVideoDecodeAccelerator by decoding an h264 data file and
rendering the resulting frames to the screen.

Fixed bugs in OmxVideoDecodeAccelerator to make this work, and removed some of
the hardcodedness of Gles2TextureToEglImageTranslator.

BUG=none
TEST=copy the generated binary to a kaen and run (as user chronos):
     DISPLAY=:0 XAUTHORITY=/home/chronos/.Xauthority LD_PRELOAD="libnvmm_service.so libnvrm_graphics_impl.so"  ./omx_video_decode_accelerator_unittest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87886

[Ami GONE FROM CHROMIUM, 2011-06-02 22:32:41]

brettw: for OWNERS

This still has some bugs:
- It produces 286 frames where the file only has 250 frames
- The rendered frames have visual artifacts / corruption issues.

However I still want to check this in soon because it should let us iterate much
faster on revisions.

[brettw, 2011-06-02 23:55:04]

LGTM owners rubber stamp

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/gles2_textur...
File content/common/gpu/gles2_texture_to_egl_image_translator.h (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/gles2_textur...
content/common/gpu/gles2_texture_to_egl_image_translator.h:30: EGLDisplay
egl_display, EGLContext egl_context, uint32 texture);
Style nit: align args if you can, only indent to 4 spaces if you can't fit them.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:534: }  // anonymous
namespace
This should just be "}  // namespace"

[Ami GONE FROM CHROMIUM, 2011-06-03 02:08:48]

vmr/Matti/Vish: I also meant to ask whether you saw either problem (visual
artifacts and/or incorrect # of frames) in the incarnations you've had of
the OmxVideoDecodeAccelerator decoding video?

[Ami GONE FROM CHROMIUM, 2011-06-03 02:13:06]

Thanks for the fast turnaround, Brett!

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/gles2_textur...
File content/common/gpu/gles2_texture_to_egl_image_translator.h (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/gles2_textur...
content/common/gpu/gles2_texture_to_egl_image_translator.h:30: EGLDisplay
egl_display, EGLContext egl_context, uint32 texture);
On 2011/06/02 23:55:04, brettw wrote:
> Style nit: align args if you can, only indent to 4 spaces if you can't fit
them.

Done.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:534: }  // anonymous
namespace
On 2011/06/02 23:55:04, brettw wrote:
> This should just be "}  // namespace"

Done.

[vrk (LEFT CHROMIUM), 2011-06-03 07:55:23]

Looking so good!

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:381: return;
?

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:386: //
TODO(vhiremath@nvidia.com) Avoid leaking of the picture buffer.
(unrelated to your change but) Out of curiosity where's the leak?

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:455: VLOG(1) << "Starting
actual flush for EOS";
DCHECK(!on_state_event_func_)?

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:502: if (port ==
output_port_) {
Instead of "return;" at 499, change this to else if...

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:507: client_state_ =
OMX_StatePause;
... and move this code into output_port block above?

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:545:
DCHECK(!on_state_event_func_);
Delete; same as 543

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.h:110: EGLContext egl_context_;
Someday, one context will not be good enough! Today though, it is. We should
also have some mechanism to handle lost contexts... maybe we should make crbugs
for these larger tasks to do?

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:118: if (width_) {
I get what you're doing, but at first glance looks a little strange. Maybe add a
comment briefly explaining.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:183: "precision
highp float; precision highp int;\n"
These shouldn't be necessary. (no ints/floats used)

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:195: "precision
mediump int;\n"
These shouldn't be necessary. (no ints/floats used)

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:280: class
ClientStateNotification {
SO ELEGANT

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:411: // (since it's
always inferrable from context).
Yeah I think for iteration 1 it'd be good to assume one context, so let's get
rid of context per texture.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:487:
ClientStateNotification note;
nit: indentation

[Paweł Hajdan Jr., 2011-06-03 09:16:18]

Drive-by with testing comments.

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
File content/common/gpu/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:99: CHECK_EQ(width_,
0) << "Must call UnInitialize before dtor.";
All those CHECKs will crash the entire test binary. This is extremely bad,
because it will prevent all following tests from running. Can you find a better
way, like using GTEST ASSERT/EXPECT macros?

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:113:
CHECK_EQ((int)glGetError(), GL_NO_ERROR);
nit: C-style casts are banned:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

Please make sure to scan the entire CL for that.

[vhiremath, 2011-06-03 11:01:19]

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
File content/common/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:532:
client_->NotifyInitializeDone();
Ami,
What is plugin's reaction to this notification ?
Does it start sending Decode() calls to OVDA ?

In that case, this notification should not be here.
The omx initialization is complete only when it has allocated input and output
buffers and omx component is in executing state and initial FillBuffers has been
sent to decoder (ensures decoder has output buffers ready to decode).
But this place does not ensure that.

It is better to move this notification to
OmxVideoDecodeAccelerator::OnStateChangeIdleToExecuting() function.


If this notification is left here itself, then this might cause hang, in case
omx component take some time to enter into execution state and plugin calls
Decode (based on NotifyInitializeDone) before the execution state change.
==========

Also, on side note: (sorry if i missed previous CLs)
there was another notification: NotifyResourcesAcquired.
why was that removed ?

[Ami GONE FROM CHROMIUM, 2011-06-03 17:43:59]

Thanks for the reviews everyone.  I've addressed all your comments below &
uploaded updated patch.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:381: return;
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> ?

In case more code is added to this method below, it's safer to explicitly
return.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:386: //
TODO(vhiremath@nvidia.com) Avoid leaking of the picture buffer.
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> (unrelated to your change but) Out of curiosity where's the leak?

I assume it's in the very next "return" two lines down.  Basically if the
decoder is not prepared to accept the picturebuffer, we don't dismiss it.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:455: VLOG(1) << "Starting
actual flush for EOS";
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> DCHECK(!on_state_event_func_)?

I like the way you think!

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:502: if (port ==
output_port_) {
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> Instead of "return;" at 499, change this to else if...

Actually, since there's no code in common between the input & output cases, I
broke them into separate functions entirely.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:507: client_state_ =
OMX_StatePause;
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> ... and move this code into output_port block above?

see above.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:545:
DCHECK(!on_state_event_func_);
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> Delete; same as 543

Done.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.h:110: EGLContext egl_context_;
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> Someday, one context will not be good enough! Today though, it is. We should
> also have some mechanism to handle lost contexts... maybe we should make
crbugs
> for these larger tasks to do?

NOTE'd the former, TODO'd the latter.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
File content/common/gpu/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:118: if (width_) {
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> I get what you're doing, but at first glance looks a little strange. Maybe add
a
> comment briefly explaining.

Done.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:183: "precision
highp float; precision highp int;\n"
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> These shouldn't be necessary. (no ints/floats used)

Whoah!  Do you actually know GL?  I don't!  Yay!

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:195: "precision
mediump int;\n"
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> These shouldn't be necessary. (no ints/floats used)

int line can be removed, but removing the float line yields:

[0603/103324:FATAL:omx_video_decode_accelerator_unittest.cc(117)] (2) : error
C7573: OpenGL/ES requires precision specifier on float types

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:411: // (since it's
always inferrable from context).
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> Yeah I think for iteration 1 it'd be good to assume one context, so let's get
> rid of context per texture.

Follow-up CL.

http://codereview.chromium.org/7088021/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:487:
ClientStateNotification note;
On 2011/06/03 07:55:24, Victoria Kirst wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
File content/common/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:532:
client_->NotifyInitializeDone();
On 2011/06/03 11:01:20, vhiremath wrote:
> Ami,
> What is plugin's reaction to this notification ?
> Does it start sending Decode() calls to OVDA ?
> 
> In that case, this notification should not be here.
> The omx initialization is complete only when it has allocated input and output
> buffers and omx component is in executing state and initial FillBuffers has
been
> sent to decoder (ensures decoder has output buffers ready to decode).
> But this place does not ensure that.
> 
> It is better to move this notification to
> OmxVideoDecodeAccelerator::OnStateChangeIdleToExecuting() function.
> 
> 
> If this notification is left here itself, then this might cause hang, in case
> omx component take some time to enter into execution state and plugin calls
> Decode (based on NotifyInitializeDone) before the execution state change.

Great catch!  Fixed.  
I was in fact seeing deadlock on startup of the test intermittently, and that's
gone now.  Excellent.

> ==========
> 
> Also, on side note: (sorry if i missed previous CLs)
> there was another notification: NotifyResourcesAcquired.
> why was that removed ? 

NotifyResourcesAcquired was replaced with NotifyInitializationDone because it
makes more sense to be symmetric with Initialize.
(the actual evolution was more contorted than that, with each step being a
positive increment over the last, but the overall effect is as above).

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
File content/common/gpu/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:99: CHECK_EQ(width_,
0) << "Must call UnInitialize before dtor.";
On 2011/06/03 09:16:18, Paweł Hajdan Jr. wrote:
> All those CHECKs will crash the entire test binary. This is extremely bad,
> because it will prevent all following tests from running. Can you find a
better
> way, like using GTEST ASSERT/EXPECT macros?

CHECKs in the test are detecting programming errors, not runtime or test
failures.  They are effectively documenting in code the way the code is meant to
be used.  In "real" code, they'd be DCHECK's, but because this is a test binary
I promoted them to CHECKs.  This is intentional.
Note that I do use ASSERT & EXPECT for the parts of the test that are actually
testing the code under test.  I only use CHECK to enforce that the test-support
code is written correctly.

http://codereview.chromium.org/7088021/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator_unittest.cc:113:
CHECK_EQ((int)glGetError(), GL_NO_ERROR);
On 2011/06/03 09:16:18, Paweł Hajdan Jr. wrote:
> nit: C-style casts are banned:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...
> 
> Please make sure to scan the entire CL for that.

Done.

[vrk (LEFT CHROMIUM), 2011-06-03 22:17:22]

LGTM!

Thanks Ami!!