Updated OMX decoder for recent PPAPI changes, and added to the build.

Updated OMX decoder for recent PPAPI changes, and added to the build.

Had to move from content/gpu to content/common/gpu to allow gpu_video_service.cc
to depend on the decoder.
Removed some dead code and did some random cleanup while I was in there.

BUG=none
TEST=chrome compiles on cros/arm!

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

[vhiremath, 2011-05-23 14:43:14]

Notify events look better over callbacks.
I just have couple of doubts as in comments.

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

http://codereview.chromium.org/7057027/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:6: 
what is the reason we are moving OVDA.cc/h to common/gpu ?
Just curious !

http://codereview.chromium.org/7057027/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:639: new
media::Picture(gles_buffer->id(), 42 /* bitstreambuffer id XXX */,
what is 42 and 24 below ?
Or, is this even required here ?
i mean is there reason why "picture" requires to know about bitstreambuffer_id ?

[Ami GONE FROM CHROMIUM, 2011-05-23 15:49:25]

> Notify events look better over callbacks.

I'm not sure if this is a vote for the old way or the new way.

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

http://codereview.chromium.org/7057027/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:6: 
On 2011/05/23 14:43:14, vhiremath wrote:
> what is the reason we are moving OVDA.cc/h to common/gpu ?
> Just curious !

Because of line 65 of
http://codereview.chromium.org/7057027/diff/1/content/common/gpu/gpu_video_se...
Namely, GpuVideoService needs to be able to set the accelerator on the decoder,
and that lives in content/common/gpu.  c/c/g may not depend on content/gpu,
hence the move.

http://codereview.chromium.org/7057027/diff/1/content/common/gpu/omx_video_de...
content/common/gpu/omx_video_decode_accelerator.cc:639: new
media::Picture(gles_buffer->id(), 42 /* bitstreambuffer id XXX */,
On 2011/05/23 14:43:14, vhiremath wrote:
> what is 42 and 24 below ?
> Or, is this even required here ?
> i mean is there reason why "picture" requires to know about bitstreambuffer_id
?

42 & 24 are magic garbage constants.
vrk@ & I both think Picture should not need to know the bitstreambufferid it
came from (it's not even a well-formed concept considering a picture can come
from bits straddling arbitrarily many bitstreambuffers!).  I expect to kill that
field in Picture shortly.

[jam, 2011-05-23 17:00:42]

content lgtm (stuff under content/gpu can be looked at with people who're in
that OWNERS file and are familiar with the code)

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

http://codereview.chromium.org/7057027/diff/1/content/common/gpu/gles2_textur...
content/common/gpu/gles2_texture_to_egl_image_translator.h:5: #ifndef
CONTENT_GPU_GLES2_TEXTURE_TO_EGL_IMAGE_TRANSLATOR_H_
nit: update guard

[Ami GONE FROM CHROMIUM, 2011-05-23 17:11:33]

Thanks John.

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

http://codereview.chromium.org/7057027/diff/1/content/common/gpu/gles2_textur...
content/common/gpu/gles2_texture_to_egl_image_translator.h:5: #ifndef
CONTENT_GPU_GLES2_TEXTURE_TO_EGL_IMAGE_TRANSLATOR_H_
On 2011/05/23 17:00:43, John Abd-El-Malek wrote:
> nit: update guard

Done.

[Ken Russell (switch to Gerrit), 2011-05-23 18:15:59]

LTGM from OWNERS standpoint modulo a few needed comment updates.

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

http://codereview.chromium.org/7057027/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:639: new
media::Picture(gles_buffer->id(), 42 /* bitstreambuffer id XXX */,
TODO?

http://codereview.chromium.org/7057027/diff/6001/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:655: sysmem_buffer->id(), 24
/* bitstreambuffer id XXX */,
TODO?

http://codereview.chromium.org/7057027/diff/6001/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/7057027/diff/6001/media/video/picture.h#newcode94
media/video/picture.h:94: // XXX what is this good for?  Absolutely nuthin'!?
Use a TODO here.

[Ken Russell (switch to Gerrit), 2011-05-23 18:18:58]

On 2011/05/23 18:15:59, kbr wrote:
> LTGM from OWNERS standpoint modulo a few needed comment updates.

Er, LGTM.

[Ami GONE FROM CHROMIUM, 2011-05-23 18:21:24]

Added TODOs.  Thanks.

[scherkus (not reviewing), 2011-05-23 21:06:06]

LGTM

[vrk (LEFT CHROMIUM), 2011-05-23 23:35:00]

Looks good! Had questions on deletion but otherwise LGTM.

http://codereview.chromium.org/7057027/diff/11002/content/common/gpu/omx_vide...
File content/common/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7057027/diff/11002/content/common/gpu/omx_vide...
content/common/gpu/omx_video_decode_accelerator.cc:545:
client_->DismissPictureBuffer(output_picture.first);
(Sorry if this was asked/answered before in a previous code review!) How is
pAppPrivate getting deleted? I would think it should be deleted here?

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/gpu_video...
File content/common/gpu/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/gpu_video...
content/common/gpu/gpu_video_decode_accelerator.cc:153: if
(!video_decode_accelerator_.get()->Flush()) {
Remote .get()

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

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:133: // NOTIMPLEMENTED().
Sounds like a good idea to me.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:294: // NOTE: this is only
partially-implemented as it only inspects the first
I don't believe the first part of this comment is relevant anymore?

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:302: base_buffers[i] = new
media::GLESBuffer(buffers[i]);
How does this get deleted? Looks like it should be deleted when you call
DismissPictureBuffer.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:311: base_buffers[i] = new
media::SysmemBuffer(buffers[i]);
Same question w/ deletion.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:640: new
media::Picture(gles_buffer->id(),
How does this get deleted? Seems like it should be deleted when you get calls to
ReusePictureBuffer. (ReusePictureBuffer will reuse the GLESBuffer or
SysmemBuffer, but the Picture data structure should be no longer needed.)

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:656: omx_buffer->pAppPrivate
= new media::Picture(
Same question w/ deletion.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
File content/common/gpu/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.h:33: void GetConfigs(const
std::vector<uint32>& requested_configs,
Can you add the OVERRIDE macro (in base/compiler_specific.h) to the end of all
of these methods? Should hopefully catch the next time we have API deviations!

[Ami GONE FROM CHROMIUM, 2011-05-24 05:31:50]

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/gpu_video...
File content/common/gpu/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/gpu_video...
content/common/gpu/gpu_video_decode_accelerator.cc:153: if
(!video_decode_accelerator_.get()->Flush()) {
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> Remote .get()

Done.

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

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:133: // NOTIMPLEMENTED().
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> Sounds like a good idea to me.

Good.  My plan is to wait until this code actually works in something checked in
(tester, sample plugin, *anything*, as long as it's checked in) and then take
the scalpel to it.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:294: // NOTE: this is only
partially-implemented as it only inspects the first
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> I don't believe the first part of this comment is relevant anymore?

Done.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:302: base_buffers[i] = new
media::GLESBuffer(buffers[i]);
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> How does this get deleted? Looks like it should be deleted when you call
> DismissPictureBuffer.

Done.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:311: base_buffers[i] = new
media::SysmemBuffer(buffers[i]);
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> Same question w/ deletion.

Done.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:640: new
media::Picture(gles_buffer->id(),
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> How does this get deleted? Seems like it should be deleted when you get calls
to
> ReusePictureBuffer. (ReusePictureBuffer will reuse the GLESBuffer or
> SysmemBuffer, but the Picture data structure should be no longer needed.)

AFAIUI this is reused between invocations (i.e. the same Picture will hold
different frames at different times), so I don't think we want to delete in
ReusePictureBuffer.
But we were indeed leaking these.  Now delete'ing in FreeOutputBuffers().

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.cc:656: omx_buffer->pAppPrivate
= new media::Picture(
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> Same question w/ deletion.

Done.

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
File content/common/gpu/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7057027/diff/6003/content/common/gpu/omx_video...
content/common/gpu/omx_video_decode_accelerator.h:33: void GetConfigs(const
std::vector<uint32>& requested_configs,
On 2011/05/23 23:35:00, Victoria Kirst wrote:
> Can you add the OVERRIDE macro (in base/compiler_specific.h) to the end of all
> of these methods? Should hopefully catch the next time we have API deviations!

Done.