Added FakeGlVideoDecodeEngine to exercise the IPC protocol for hardware video decoding

Added FakeGlVideoDecodeEngine to exercise the IPC protocol for hardware video decoding

There are several things done in this patch:
1. Added FakeGlVideoDecodeEngine
2. Fixed style problem in VideoDecodeEngine and gpu_video_common.h
3. Added route to pass texture from gpu process to WebKit

BUG=53714
TEST=Tree is green

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

[Alpha Left Google, 2010-09-09 02:04:11]

Note that the way textures are passed around is really hacky now. I'm working on
another patch that cleans the procedure for allocating output buffers.

[jiesun, 2010-09-09 17:19:08]

you had missed some files, I guess.

also I am concerned if you could always sent outside texture to decoder, have
you tried to feed the MFT with D3DSurface9?

where is the fake texture id get mapped to real texture id?

http://codereview.chromium.org/3335014/diff/1/4
File chrome/common/gpu_video_common.h (right):

http://codereview.chromium.org/3335014/diff/1/4#newcode85
chrome/common/gpu_video_common.h:85: // to be consumed. Before that we need API
to establish the buffers.
not sure how could that be done in all cases

http://codereview.chromium.org/3335014/diff/1/6
File chrome/gpu/gpu_video_decoder.cc (right):

http://codereview.chromium.org/3335014/diff/1/6#newcode110
chrome/gpu/gpu_video_decoder.cc:110: info.stream_info.surface_type);
little tricky that two enums had to be defined with same order.

http://codereview.chromium.org/3335014/diff/1/6#newcode192
chrome/gpu/gpu_video_decoder.cc:192: output_param.texture =
frame->gl_texture(media::VideoFrame::kRGBPlane);
if this is true, maybe we should define texture as array?

http://codereview.chromium.org/3335014/diff/1/6#newcode210
chrome/gpu/gpu_video_decoder.cc:210: // TODO(jiesun): find a better way to
determine which GpuVideoDecoder
the comment is not updated, please say "which engine"...

http://codereview.chromium.org/3335014/diff/1/6#newcode257
chrome/gpu/gpu_video_decoder.cc:257: DCHECK(ret) << "Failed to switch context";
which thread will this running? I guess we plan to have a separate threads for
each decoder and do not mess up with gpu main thread. and also because MFT is
not always support in async mode. if that 's the case, I seriously doubt this
MakeCurrent will success. because gpu command buffer stub could be make it
current in another thread.
at least this will fail on tegra.

http://codereview.chromium.org/3335014/diff/1/6#newcode278
chrome/gpu/gpu_video_decoder.cc:278: decode_engine_->ProduceVideoFrame(frame);
not sure why we allocate texture each time when OnFilLThisBuffer is called.
could we do this once and recycle it?

http://codereview.chromium.org/3335014/diff/1/12
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3335014/diff/1/12#newcode77
chrome/renderer/media/ipc_video_decoder.cc:77: GL_UNSIGNED_BYTE, NULL);
why it had to be RGBA packed format? why you assume the width height provide
here is enough for decode, i.e alignment issue?

http://codereview.chromium.org/3335014/diff/1/15
File media/base/video_frame.h (right):

http://codereview.chromium.org/3335014/diff/1/15#newcode54
media/base/video_frame.h:54: typedef unsigned int GlTexture;
seems to me that private_buffer_ should be removed.

http://codereview.chromium.org/3335014/diff/1/20
File media/video/video_decode_engine.h (right):

http://codereview.chromium.org/3335014/diff/1/20#newcode60
media/video/video_decode_engine.h:60: uint32 surface_height;
because of this, if you create a texture in client and passed in the decoder, it
may not able to use it, because it may require surface_height = align_up(height,
16 or 64)...

[Alpha Left Google, 2010-09-09 19:10:58]

http://codereview.chromium.org/3335014/diff/1/4
File chrome/common/gpu_video_common.h (right):

http://codereview.chromium.org/3335014/diff/1/4#newcode85
chrome/common/gpu_video_common.h:85: // to be consumed. Before that we need API
to establish the buffers.
On 2010/09/09 17:19:08, jiesun wrote:
> not sure how could that be done in all cases

I don't know what you mean by all cases. You just need negotiation for buffer
allocation and then you'll get buffer id or even simpler you can just use a
texture id that is the  same as the one used in command buffer.

http://codereview.chromium.org/3335014/diff/1/6
File chrome/gpu/gpu_video_decoder.cc (right):

http://codereview.chromium.org/3335014/diff/1/6#newcode110
chrome/gpu/gpu_video_decoder.cc:110: info.stream_info.surface_type);
On 2010/09/09 17:19:08, jiesun wrote:
> little tricky that two enums had to be defined with same order.

We could use compile assert to make sure they are the same. Will do that next
round.

http://codereview.chromium.org/3335014/diff/1/6#newcode192
chrome/gpu/gpu_video_decoder.cc:192: output_param.texture =
frame->gl_texture(media::VideoFrame::kRGBPlane);
On 2010/09/09 17:19:08, jiesun wrote:
> if this is true, maybe we should define texture as array?

What I mean is that we need an extra layer to convert between VideoFrame and
something we can transport in IPC. I'll need to define something like a
GpuVideoDecodeContext that provide extra methods to do this.

http://codereview.chromium.org/3335014/diff/1/6#newcode210
chrome/gpu/gpu_video_decoder.cc:210: // TODO(jiesun): find a better way to
determine which GpuVideoDecoder
On 2010/09/09 17:19:08, jiesun wrote:
> the comment is not updated, please say "which engine"...

Done.

http://codereview.chromium.org/3335014/diff/1/6#newcode257
chrome/gpu/gpu_video_decoder.cc:257: DCHECK(ret) << "Failed to switch context";
On 2010/09/09 17:19:08, jiesun wrote:
> which thread will this running? I guess we plan to have a separate threads for
> each decoder and do not mess up with gpu main thread. and also because MFT is
> not always support in async mode. if that 's the case, I seriously doubt this
> MakeCurrent will success. because gpu command buffer stub could be make it
> current in another thread.
> at least this will fail on tegra. 

There's only one main thread in GPU process. This runs on the same thread as the
gpu decoder. Going forward we'll have one thread for all GpuVideoDecoder, it
makes no sense to have multiple threads since all messages come from the main
thread. If mft needs to run a separate thread it will do so internally, or we
give it a message loop. This is really temporary.

http://codereview.chromium.org/3335014/diff/1/6#newcode278
chrome/gpu/gpu_video_decoder.cc:278: decode_engine_->ProduceVideoFrame(frame);
On 2010/09/09 17:19:08, jiesun wrote:
> not sure why we allocate texture each time when OnFilLThisBuffer is called.
> could we do this once and recycle it?

Again this is temporary, the whole point of this patch is to get the
FakeGlVideoDecodeEngine in, this part of the code will be changed significantly.

http://codereview.chromium.org/3335014/diff/1/12
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3335014/diff/1/12#newcode77
chrome/renderer/media/ipc_video_decoder.cc:77: GL_UNSIGNED_BYTE, NULL);
On 2010/09/09 17:19:08, jiesun wrote:
> why it had to be RGBA packed format? why you assume the width height provide
> here is enough for decode, i.e alignment issue?

I have some comments in the .h file, this again is temporary. Like I said in the
comment, we should allocate textures based on requests from gpu process, not
done only once. This is due to the fact that the IPC commands is lacking this
function, it'll be done properly next round.

http://codereview.chromium.org/3335014/diff/1/15
File media/base/video_frame.h (right):

http://codereview.chromium.org/3335014/diff/1/15#newcode54
media/base/video_frame.h:54: typedef unsigned int GlTexture;
On 2010/09/09 17:19:08, jiesun wrote:
> seems to me that private_buffer_ should be removed.

It still has use. Like you are passing GL texture in VideoFrame for OpenMAX but
you still want to pass EGL image for it to use.

http://codereview.chromium.org/3335014/diff/1/20
File media/video/video_decode_engine.h (right):

http://codereview.chromium.org/3335014/diff/1/20#newcode60
media/video/video_decode_engine.h:60: uint32 surface_height;
On 2010/09/09 17:19:08, jiesun wrote:
> because of this, if you create a texture in client and passed in the decoder,
it
> may not able to use it, because it may require surface_height =
align_up(height,
> 16 or 64)...

You don't need to change the texture size, you can change the upload instead. I
don't think it's big enough a problem to concern now.

[Alpha Left Google, 2010-09-09 19:12:59]

On 2010/09/09 17:19:08, jiesun wrote:
> you had missed some files, I guess.
> 
> also I am concerned if you could always sent outside texture to decoder, have
> you tried to feed the MFT with D3DSurface9?

Don't understand your question. I tried copying d3dsurface9 to d3dtexture9 and
composited correctly. Uploading to a texture should always be possible, it's
just a matter of switching context and using the right texture id.

> 
> where is the fake texture id get mapped to real texture id?

Translation not done in this patch yet.

> 
> http://codereview.chromium.org/3335014/diff/1/4
> File chrome/common/gpu_video_common.h (right):
> 
> http://codereview.chromium.org/3335014/diff/1/4#newcode85
> chrome/common/gpu_video_common.h:85: // to be consumed. Before that we need
API
> to establish the buffers.
> not sure how could that be done in all cases
> 
> http://codereview.chromium.org/3335014/diff/1/6
> File chrome/gpu/gpu_video_decoder.cc (right):
> 
> http://codereview.chromium.org/3335014/diff/1/6#newcode110
> chrome/gpu/gpu_video_decoder.cc:110: info.stream_info.surface_type);
> little tricky that two enums had to be defined with same order.
> 
> http://codereview.chromium.org/3335014/diff/1/6#newcode192
> chrome/gpu/gpu_video_decoder.cc:192: output_param.texture =
> frame->gl_texture(media::VideoFrame::kRGBPlane);
> if this is true, maybe we should define texture as array?
> 
> http://codereview.chromium.org/3335014/diff/1/6#newcode210
> chrome/gpu/gpu_video_decoder.cc:210: // TODO(jiesun): find a better way to
> determine which GpuVideoDecoder
> the comment is not updated, please say "which engine"...
> 
> http://codereview.chromium.org/3335014/diff/1/6#newcode257
> chrome/gpu/gpu_video_decoder.cc:257: DCHECK(ret) << "Failed to switch
context";
> which thread will this running? I guess we plan to have a separate threads for
> each decoder and do not mess up with gpu main thread. and also because MFT is
> not always support in async mode. if that 's the case, I seriously doubt this
> MakeCurrent will success. because gpu command buffer stub could be make it
> current in another thread.
> at least this will fail on tegra.
> 
> http://codereview.chromium.org/3335014/diff/1/6#newcode278
> chrome/gpu/gpu_video_decoder.cc:278: decode_engine_->ProduceVideoFrame(frame);
> not sure why we allocate texture each time when OnFilLThisBuffer is called.
> could we do this once and recycle it?
> 
> http://codereview.chromium.org/3335014/diff/1/12
> File chrome/renderer/media/ipc_video_decoder.cc (right):
> 
> http://codereview.chromium.org/3335014/diff/1/12#newcode77
> chrome/renderer/media/ipc_video_decoder.cc:77: GL_UNSIGNED_BYTE, NULL);
> why it had to be RGBA packed format? why you assume the width height provide
> here is enough for decode, i.e alignment issue?
> 
> http://codereview.chromium.org/3335014/diff/1/15
> File media/base/video_frame.h (right):
> 
> http://codereview.chromium.org/3335014/diff/1/15#newcode54
> media/base/video_frame.h:54: typedef unsigned int GlTexture;
> seems to me that private_buffer_ should be removed.
> 
> http://codereview.chromium.org/3335014/diff/1/20
> File media/video/video_decode_engine.h (right):
> 
> http://codereview.chromium.org/3335014/diff/1/20#newcode60
> media/video/video_decode_engine.h:60: uint32 surface_height;
> because of this, if you create a texture in client and passed in the decoder,
it
> may not able to use it, because it may require surface_height =
align_up(height,
> 16 or 64)...

[Alpha Left Google, 2010-09-09 20:01:34]

Add fake_gl_video_decode_engine.{cc, h}

[jiesun, 2010-09-10 17:30:21]

is this patch able to run and let us see the random pattern somehow, or it need
other patches to land?

if you only want to fake engine checked in, I am okay with that. but other
things is just confusing, at least for me.

http://codereview.chromium.org/3335014/diff/13001/14005
File chrome/gpu/gpu_video_decoder.cc (right):

http://codereview.chromium.org/3335014/diff/13001/14005#newcode217
chrome/gpu/gpu_video_decoder.cc:217: 
this comments say you want to initialize this in gpu process.
I am confused about the plan.

http://codereview.chromium.org/3335014/diff/13001/14007
File chrome/gpu/media/fake_gl_video_decode_engine.cc (right):

http://codereview.chromium.org/3335014/diff/13001/14007#newcode86
chrome/gpu/media/fake_gl_video_decode_engine.cc:86:
handler_->ConsumeVideoFrame(frame);
time stamp?

http://codereview.chromium.org/3335014/diff/13001/14012
File chrome/renderer/media/gles2_video_decode_context.h (right):

http://codereview.chromium.org/3335014/diff/13001/14012#newcode13
chrome/renderer/media/gles2_video_decode_context.h:13: }  // namespace ggl
If you use this, I assume instance of this class will live in renderer process.
which is contradictory with other comments in this patch.

[Alpha Left Google, 2010-09-10 18:33:06]

On 2010/09/10 17:30:21, jiesun wrote:
> is this patch able to run and let us see the random pattern somehow, or it
need
> other patches to land?

It still WebKit patches and additional patches to run.

> 
> if you only want to fake engine checked in, I am okay with that. but other
> things is just confusing, at least for me.

This is mostly proof-of-concept, and mostly documenting in the code what I
should do. I think the comments should give you an idea how things will look
like.

> 
> http://codereview.chromium.org/3335014/diff/13001/14005
> File chrome/gpu/gpu_video_decoder.cc (right):
> 
> http://codereview.chromium.org/3335014/diff/13001/14005#newcode217
> chrome/gpu/gpu_video_decoder.cc:217: 
> this comments say you want to initialize this in gpu process.
> I am confused about the plan.

Check with other patch about GpuVideoDecodeContext. It is explained there.

> 
> http://codereview.chromium.org/3335014/diff/13001/14007
> File chrome/gpu/media/fake_gl_video_decode_engine.cc (right):
> 
> http://codereview.chromium.org/3335014/diff/13001/14007#newcode86
> chrome/gpu/media/fake_gl_video_decode_engine.cc:86:
> handler_->ConsumeVideoFrame(frame);
> time stamp?
> 
> http://codereview.chromium.org/3335014/diff/13001/14012
> File chrome/renderer/media/gles2_video_decode_context.h (right):
> 
> http://codereview.chromium.org/3335014/diff/13001/14012#newcode13
> chrome/renderer/media/gles2_video_decode_context.h:13: }  // namespace ggl
> If you use this, I assume instance of this class will live in renderer
process.
> which is contradictory with other comments in this patch.


I don't see how contradictory it is, ggl context IS in the renderer process.
Gles2VideoDecodeContext IS running inside the renderer process. See comments in
this file.

[jiesun, 2010-09-10 19:47:12]

Okay, I was expecting to see "proofs" for this "proof-of-concept" patch.:) Could
you please point the other CLs related to this? so I could get better
understanding.
Putting Gles2 video decoder context inside this CL gave me an impression that
this video decoder context will be used with FakexxxEngine and GpuVideoDecoder.
Only after seeing another CL it turns out that GpuVideoDecodeContext maybe is
what is mentioned in this CL. Confusing? :) The naming of the derived classes
are also not helping. 
And finally part of the concept of allocator(Commit/Decommit) creep back under
the name of "VideoDecodeContext". :)
BTW, why we need a GpuVideoDecodeContext as abstract class, the interface is
little awkward (such as the parameter n cross the interface functions)? we could
make it concrete and pass VideoDecodeContext* to engine?

Also please address the issue of how to fake the timestamp too, otherwise you
probably only will see one frame.

[Alpha Left Google, 2010-09-10 20:01:31]

On 2010/09/10 19:47:12, jiesun wrote:
> Okay, I was expecting to see "proofs" for this "proof-of-concept" patch.:)
Could
> you please point the other CLs related to this? so I could get better
> understanding.

I already said it, it's in GpuVideoDecodeContext patch. And there's already tons
of comments in Gles2VideoDecodeContext. 

> Putting Gles2 video decoder context inside this CL gave me an impression that
> this video decoder context will be used with FakexxxEngine and
GpuVideoDecoder.

I wrote more comments than code in Gles2VideoDecodeContext, I don't see how
confusing it is. Please read the commands in the code. It is extremely clear
that it is used inside the renderer process along with the GLES2 context in the
renderer.

Changes in Gles2VideoDecodeContext is so that we can connect context in renderer
to it.

> Only after seeing another CL it turns out that GpuVideoDecodeContext maybe is
> what is mentioned in this CL. Confusing? :) The naming of the derived classes
> are also not helping. 

Everything inside the gpu process starts with GpuXXX. Gles2VideoDecodeContext is
working with the GLES2 context. I don't see better names than that.

> And finally part of the concept of allocator(Commit/Decommit) creep back under
> the name of "VideoDecodeContext". :)
> BTW, why we need a GpuVideoDecodeContext as abstract class, the interface is
> little awkward (such as the parameter n cross the interface functions)? we
could
> make it concrete and pass VideoDecodeContext* to engine?

We implement concrete class and give it to VideoDecodeContext.

> 
> Also please address the issue of how to fake the timestamp too, otherwise you
> probably only will see one frame.

Will address that in next patch. I added a todo in the code.

[jiesun, 2010-09-10 20:51:03]

On 2010/09/10 20:01:31, Alpha wrote:
> On 2010/09/10 19:47:12, jiesun wrote:
> > Okay, I was expecting to see "proofs" for this "proof-of-concept" patch.:)
> Could
> > you please point the other CLs related to this? so I could get better
> > understanding.
> 
> I already said it, it's in GpuVideoDecodeContext patch. And there's already
tons
> of comments in Gles2VideoDecodeContext. 
> 
It seems that I am not reviewer for that patch. and I did look at the comments
Gles2VideoDecodeContext, it mention both software and hardware decode engine,
therefore it logically follow that you will had D3DVideoDecodeContext as
parallel (for D3D backend?)  instead of use Gles2** for software and Gpu** for
hardware (is that what you what? ). 
> > Putting Gles2 video decoder context inside this CL gave me an impression
that
> > this video decoder context will be used with FakexxxEngine and
> GpuVideoDecoder.
> 
> I wrote more comments than code in Gles2VideoDecodeContext, I don't see how
> confusing it is. Please read the commands in the code. It is extremely clear
> that it is used inside the renderer process along with the GLES2 context in
the
> renderer.
> 
> Changes in Gles2VideoDecodeContext is so that we can connect context in
renderer
> to it.
I understand this, I just say putting irrelevant change into one patch is
confusing.
> 
> > Only after seeing another CL it turns out that GpuVideoDecodeContext maybe
is
> > what is mentioned in this CL. Confusing? :) The naming of the derived
classes
> > are also not helping. 
> 
> Everything inside the gpu process starts with GpuXXX. Gles2VideoDecodeContext
is
> working with the GLES2 context. I don't see better names than that.

In my understanding, GLES2 or subset of it(WebGL etc.) is used as
front-end(inside renderer process), D3D/GLES2/GL/Mesa is used as backend(inside
gpu process).  The name of "GLES2" unfortunately used in both side, which is the
begining of the confusion, especially you said in those comments in
Gles2VideoDecodeEngine that is used when Gles2 as a "backend" and
Gles2VideoDecodeEngine will handle both hardware and software case. I think that
you may need to update the comments in that file. sounds fair?

> 
> > And finally part of the concept of allocator(Commit/Decommit) creep back
under
> > the name of "VideoDecodeContext". :)
> > BTW, why we need a GpuVideoDecodeContext as abstract class, the interface is
> > little awkward (such as the parameter n cross the interface functions)? we
> could
> > make it concrete and pass VideoDecodeContext* to engine?
> 
> We implement concrete class and give it to VideoDecodeContext.
Will decode engine see GpuVideoDecodeContext* or just VideoDecodeContext*?

> 
> > 
> > Also please address the issue of how to fake the timestamp too, otherwise
you
> > probably only will see one frame.
> 
> Will address that in next patch. I added a todo in the code.

[Alpha Left Google, 2010-09-10 21:57:19]

On 2010/09/10 20:51:03, jiesun wrote:
> On 2010/09/10 20:01:31, Alpha wrote:
> > On 2010/09/10 19:47:12, jiesun wrote:
> > > Okay, I was expecting to see "proofs" for this "proof-of-concept" patch.:)
> > Could
> > > you please point the other CLs related to this? so I could get better
> > > understanding.
> > 
> > I already said it, it's in GpuVideoDecodeContext patch. And there's already
> tons
> > of comments in Gles2VideoDecodeContext. 
> > 
> It seems that I am not reviewer for that patch. and I did look at the comments
> Gles2VideoDecodeContext, it mention both software and hardware decode engine,
> therefore it logically follow that you will had D3DVideoDecodeContext as
> parallel (for D3D backend?)  instead of use Gles2** for software and Gpu** for
> hardware (is that what you what? ). 

Gles2VideoDecodeContext is in the renderer, in the renderer there will only be
GLES2 so no there won't be a D3dVideoDecodeContext in the renderer. Everything
the renderer see is GL textures.

> > > Putting Gles2 video decoder context inside this CL gave me an impression
> that
> > > this video decoder context will be used with FakexxxEngine and
> > GpuVideoDecoder.
> > 
> > I wrote more comments than code in Gles2VideoDecodeContext, I don't see how
> > confusing it is. Please read the commands in the code. It is extremely clear
> > that it is used inside the renderer process along with the GLES2 context in
> the
> > renderer.
> > 
> > Changes in Gles2VideoDecodeContext is so that we can connect context in
> renderer
> > to it.
> I understand this, I just say putting irrelevant change into one patch is
> confusing.

Sorry about that, I should have separated that.

> > 
> > > Only after seeing another CL it turns out that GpuVideoDecodeContext maybe
> is
> > > what is mentioned in this CL. Confusing? :) The naming of the derived
> classes
> > > are also not helping. 
> > 
> > Everything inside the gpu process starts with GpuXXX.
Gles2VideoDecodeContext
> is
> > working with the GLES2 context. I don't see better names than that.
> 
> In my understanding, GLES2 or subset of it(WebGL etc.) is used as
> front-end(inside renderer process), D3D/GLES2/GL/Mesa is used as
backend(inside
> gpu process).  The name of "GLES2" unfortunately used in both side, which is
the
> begining of the confusion, especially you said in those comments in
> Gles2VideoDecodeEngine that is used when Gles2 as a "backend" and
> Gles2VideoDecodeEngine will handle both hardware and software case. I think
that
> you may need to update the comments in that file. sounds fair?

Gles2VideoDecodeContext inside the renderer I think it's fair. I removed the
reference to GLES2 context in the comments if that helps you understand.

GLES2 is the front end. There's a context associated with a GLES2 Command
Decoder, that's what I'm referring to in the comments. It can be mesa, gles2 or
gl.

> 
> > 
> > > And finally part of the concept of allocator(Commit/Decommit) creep back
> under
> > > the name of "VideoDecodeContext". :)
> > > BTW, why we need a GpuVideoDecodeContext as abstract class, the interface
is
> > > little awkward (such as the parameter n cross the interface functions)? we
> > could
> > > make it concrete and pass VideoDecodeContext* to engine?
> > 
> > We implement concrete class and give it to VideoDecodeContext.
> Will decode engine see GpuVideoDecodeContext* or just VideoDecodeContext*?

VideoDecodeEngine sees only VideoDecodeContext. See comments in my other patch
about GpuVideoDecodeContext.

> 
> > 
> > > 
> > > Also please address the issue of how to fake the timestamp too, otherwise
> you
> > > probably only will see one frame.
> > 
> > Will address that in next patch. I added a todo in the code.

[jiesun, 2010-09-10 22:55:11]

LGTM if you fix those comments and certainly bots really go green. 

I prefer change name of the Gles2VideoDecodeContext to something more
meaningful, in later CLs if you want.
I still not sure about GpuVideoDecodeContext stuff, what a VideoDecodeContext
really do in that context. but let's see.

http://codereview.chromium.org/3335014/diff/10002/4013
File chrome/renderer/media/gles2_video_decode_context.h (right):

http://codereview.chromium.org/3335014/diff/10002/4013#newcode17
chrome/renderer/media/gles2_video_decode_context.h:17: // This is a class that
provides a video decode context using a GLES2 backend.
front end?

http://codereview.chromium.org/3335014/diff/10002/4013#newcode19
chrome/renderer/media/gles2_video_decode_context.h:19: // in the GLES2 context.
maybe ggl::Context binding?

http://codereview.chromium.org/3335014/diff/10002/4013#newcode45
chrome/renderer/media/gles2_video_decode_context.h:45: // 2. RGBA texture (aka
hardware decoding mode).
maybe we should discuss these two possibilities in the base abstract class
instead here. WDYT?

[Alpha Left Google, 2010-09-11 02:00:28]

http://codereview.chromium.org/3335014/diff/10002/4013
File chrome/renderer/media/gles2_video_decode_context.h (right):

http://codereview.chromium.org/3335014/diff/10002/4013#newcode17
chrome/renderer/media/gles2_video_decode_context.h:17: // This is a class that
provides a video decode context using a GLES2 backend.
On 2010/09/10 22:55:11, jiesun wrote:
> front end?

GLES2 is the backend inside the renderer process. I just call it ggl::Context
then.

http://codereview.chromium.org/3335014/diff/10002/4013#newcode19
chrome/renderer/media/gles2_video_decode_context.h:19: // in the GLES2 context.
On 2010/09/10 22:55:11, jiesun wrote:
> maybe ggl::Context binding?

removed gles2 context.

http://codereview.chromium.org/3335014/diff/10002/4013#newcode45
chrome/renderer/media/gles2_video_decode_context.h:45: // 2. RGBA texture (aka
hardware decoding mode).
On 2010/09/10 22:55:11, jiesun wrote:
> maybe we should discuss these two possibilities in the base abstract class
> instead here. WDYT?

The base class is for a general design. This is a specific implementation. I
might just split these into two classes, but I'll go over it laster when I work
on renderer, working on gpu process first.

[scherkus (not reviewing), 2010-09-13 07:19:22]

LGTM w/ nits

thanks for fixing style stuff, but next time try to keep those patches separate
from patches that introduce/change code

http://codereview.chromium.org/3335014/diff/4026/35007
File chrome/gpu/media/fake_gl_video_decode_engine.cc (right):

http://codereview.chromium.org/3335014/diff/4026/35007#newcode66
chrome/gpu/media/fake_gl_video_decode_engine.cc:66: static int seed = 0;
just use a uint8 and let it overflow?

but to be honest I don't really care -- as long as pixels go up on the screen :)

http://codereview.chromium.org/3335014/diff/4026/35008
File chrome/gpu/media/fake_gl_video_decode_engine.h (right):

http://codereview.chromium.org/3335014/diff/4026/35008#newcode35
chrome/gpu/media/fake_gl_video_decode_engine.h:35: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/3335014/diff/4026/35016
File media/base/video_frame.h (right):

http://codereview.chromium.org/3335014/diff/4026/35016#newcode172
media/base/video_frame.h:172: uint8* data_[kMaxPlanes];
at some point we may want to consider using a union to join
data_/gl_textures_/d3d_textures_ together to reduce the size of a video frame
class

same with private_buffer_ as well

add a TODO, perhaps?