media: gpu process ipc video decoder implementation

1. ipc_video_decoder.cc/h is media pipeline filter which use the gpu decoder facilities in video stack. it is only enabled when (a) hardware composition is on (b) hardware decoding command line is on (c) h264 codec is specified.

2. gpu_video_service.cc/h is a singleton in gpu process which provide video services for renderer process, through it we could create decoder. ( in my imagination, in the future, we could create encoder or capturer too)

3. gpu_video_decoder.cc/h. abstract interface for hardware decoder. 

4. gpu_video_service_host.cc/h is singleton in renderer process which provide proxy for gpu_video_service.

5. gpu_video_decoder_host.cc/h is proxy for gpu_video_decoder. (1 to 1 map).basically there is one global GpuVideoService in GPU process, one GpuVideoServiceHost in Renderer process. for each renderer process, there are could be multiple renderer view, each could had multiple GpuVideoDecoderHost the connect to GpuVideoDeocder through GPUCHannelHOst/GpuChannel. 

6. gpu_video_common.cc/h: IPC message definition and pickle/marshaling support.

ISSUES:

1. in media pipeline, we need let decoder to determine if bit stream filter should be used instead of let command line to determine it.
2. stop readback from D3D surface use ANGLE.
3. Flush logic still need fine tuning.
4. CreateThread in GpuVideoDecoder, and post message in message handler, and derived classs handle message loop. ?
5. Error handling.
6. Input ring buffer implementation. Current impl is naive.
7.Add output queue for MFT decoder.
8. Query Capabilities at GetVideoServices()...

BUG=None
TEST=Windows7

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

[jiesun, 2010-08-03 16:40:47]

Please review.

Please keep in mind that we are not going to finalize this in one patch. if it
is good enough for initial version , then we could unblock other people who also
want to work in this area.

to try it out, find a windows 7 set up , apply this patch.
use --enable-accelerated-composition --enable--accelerated-decoding
--use-gl=desktop.

ANGLE is not working on windows the last time I checked.

[Alpha Left Google, 2010-08-03 19:34:01]

I haven't looked into the IPC part, but here's some high level comments about
the video decoder implementation in chrome/gpu.

1. Try to make GpuVideoDecoder platform independent. There are some logic in
GpuVideoDecoderMft that can be shared, like allocation of input buffer and
sharing the memory with the renderer. Can we have something similar to how we
implement audio output? We have all the platform implementation in media/audio
and we implement the logic of using audio io and shared memory in
chrome/browser/renderer_host/audio_renderer_host.{cc, h}.

2. Unfork the media foundation implementation.
gpu_video_decoder_mft.{cc, h} has to lot to share with the media foundation
implementation in media/media_foundation/h264mft.cc that we are working on. The
implementation you have added flush support, but much of other logic are the
same. Would it makes more sense to improve  h264mft.cc instead of forking it?
I'd suggest that you check in a universal hardware video decoder interface into
media/, maybe media/hw_video_decoder and then we can move h264mft.cc to your
interface?

[scherkus (not reviewing), 2010-08-03 19:56:40]

we've got to do some restructuring as you have media code depending on chrome
code

the code should probably be set up similar to how we implemented audio:
  1) /media/audio/ contains platform-abstracted code
  2) /media/audio/win contains the windows implementation (or you can use the
_win.cc suffix)
  3) /chrome/browser/renderer_host/audio_sync_reader.* packages up /media/audio
for use in Chrome IPC
  4) /chrome/browser/renderer_host/audio_renderer_host.* uses AudioSyncReader to
create the browser-side of the IPC
  5) /chrome/renderer/audio_message_filter.* is the renderer-side of the IPC
  6) /chrome/media/audio_renderer_impl.* is the AudioRenderer implementation
that talks to AudioMessageFilter

looking at the code you have now, we would end up with something like:
  1) move /chrome/gpu/gpu_video_decoder* files to /media (instead of forking the
code there try to improve it -- this should be a separate CL)
  2) move /media/filters/ipc_video_decoder.* to /chrome/renderer/media (you'll
see we already have an experimental ipc_video_renderer.* there)
  3) move /chrome/common/chrome_switches.* change to
/media/base/media_switches.*

the rest of the stuff looks like its in the right place -- I'll try to do a pass
on that

http://codereview.chromium.org/2873089/diff/38003/45029
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/2873089/diff/38003/45029#newcode11
media/filters/ffmpeg_demuxer.cc:11: #include "chrome/common/chrome_switches.h"
media cannot depend on chrome

http://codereview.chromium.org/2873089/diff/38003/45031
File media/filters/ipc_video_decoder.h (right):

http://codereview.chromium.org/2873089/diff/38003/45031#newcode9
media/filters/ipc_video_decoder.h:9: #include
"chrome/renderer/gpu_video_service_host.h"
media cannot depend on chrome

[jiesun, 2010-08-03 21:10:17]

Moving ipc_video_decoder.cc/h to chrome/renderer/media mean add dependency of
ffmpeg to chrome renderer, is that what we want at all?

[scherkus (not reviewing), 2010-08-03 21:44:22]

On 2010/08/03 21:10:17, jiesun wrote:
> Moving ipc_video_decoder.cc/h to chrome/renderer/media mean add dependency of
> ffmpeg to chrome renderer, is that what we want at all?

Looking at your usage it's OK in this case.

The root issue is that DemuxerStream::QueryInterface() leaks the FFmpeg demuxer
abstraction, which makes your ipc_video_decoder.* depend on FFmpeg in order to
read the CodecID and AVRational time base.  MediaFormat was intended to abstract
these details, but someone added QueryInterface() as a cheesy workaround and
that workaround has persisted to this day.

There's been a long standing task to clean up the abstractions and replace
QueryInterface/MediaFormat with basic accessors (i.e., you simply call a
DemuxerStream method to get the codec and time base).

[scherkus (not reviewing), 2010-08-03 22:44:42]

Also there are no unit tests.. I would strongly encourage you to write some
after we work out where all the code will go

[jiesun, 2010-08-03 23:03:19]

how to fix the "check deps" error on linux bot?

ERROR in /b/slave/linux/build/src/chrome/renderer/media/ipc_video_decoder.cc
Illegal include: "media/ffmpeg/ffmpeg_common.h"
    Because of no rule applying
Illegal include: "media/ffmpeg/ffmpeg_util.h"
    Because of no rule applying

[scherkus (not reviewing), 2010-08-03 23:06:34]

Two ways:
  1) Fix the root issue by not using QueryInterface
  2) Adjust the rules in chrome/renderer/DEPS

I would strongly prefer (1) but if you're going to do (2) then please file a bug
(Area-WebKit, Feature-Media) and add TODOs in code as necessary

[jiesun, 2010-08-04 15:36:17]

I tried to remove QueryInterface: It seems that I only need width and height and
codec id.
I need to move kFFMPEGVideo key from ffmpeg_common.h/cc to media_format.cc/h, to
remove ffmpeg dependency.
I add width/height key in media_format() of demux_stream class. the only thing I
could not figure out is that codec_id enum value is defined in ffmpeg headers,
change that will involve a lot of files. therefore I had to leave (1) for
another patch.

[fbarchard, 2010-08-04 15:52:42]

At some point we want aspect ratio, which is a scaled width and
height.  Keep that in mind for any movie dimension queries.

On Wednesday, August 4, 2010,  <jiesun@chromium.org> wrote:
> I tried to remove QueryInterface: It seems that I only need width and height
and
> codec id.
> I need to move kFFMPEGVideo key from ffmpeg_common.h/cc to media_format.cc/h,
to
> remove ffmpeg dependency.
> I add width/height key in media_format() of demux_stream class. the only thing
I
> could not figure out is that codec_id enum value is defined in ffmpeg headers,
> change that will involve a lot of files. therefore I had to leave (1) for
> another patch.
>
> http://codereview.chromium.org/2873089/show
>

[jiesun, 2010-08-04 16:01:57]

IIRC, SAR is encoded inside elementary stream (SEI?), therefore I doubt we need
that information from demuxer.
and in current pipeline ,we always assume square samples. we also had no concept
of "crop" region too.

[jiesun, 2010-08-05 16:18:58]

ping!

[Alpha Left Google, 2010-08-05 18:55:11]

Brett: Can you look into the IPC part?

Here's my major concerns for the media part (sort by importance):
1. Really need to unfork MFT decoder
   We can't two similar files sitting around the code base. I don't see why we
can't improve existing code and needed to fork.

2. Separate out a platform independent video decoder code interface. I mean
gpu_video_decoder.h should be in media and has absolutely no information about
IPC. The IPC code in chrome/gpu will just those code.

If you insist checking this in, we have to clean it up in the next patch as we
cannot leave temporary code in the code base more than it should.

http://codereview.chromium.org/2873089/diff/69005/52020
File chrome/chrome_renderer.gypi (right):

http://codereview.chromium.org/2873089/diff/69005/52020#newcode21
chrome/chrome_renderer.gypi:21: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
I don't think this is needed in the renderer.

[brettw, 2010-08-05 21:22:51]

I didn't really look at the Windows video code, I assume the other reviewers
covered that.

http://codereview.chromium.org/2873089/diff/69005/52021
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/2873089/diff/69005/52021#newcode235
chrome/common/gpu_messages_internal.h:235:
IPC_MESSAGE_ROUTED1(GpuVideoDecoderMsg_Initialize,
You mixed the two classes of messages (renderer->gpu and gpu->renderer) into one
message group. Generally we would separate these to make it more clear and name
them accordingly. So the messages to the host would start with
GpuVideoDecoderHostMsg and the messages back would be named like you did the
ones here. Can you split these apart into two IPC_BEGIN_MESSAGES(...) groups
named appropriately?

http://codereview.chromium.org/2873089/diff/69005/52021#newcode237
chrome/common/gpu_messages_internal.h:237: // From Gpu to Render process:
confirm GpuVideoDecoder are ready or error.
Can you format these like the other messages with a blank line in between each
one?

http://codereview.chromium.org/2873089/diff/69005/52024
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52024#newcode184
chrome/gpu/gpu_channel.cc:184: // TODO(jiesun): Add Windows 7 check.
Can you expand on this TODO, since I don't really know what you're saying. I
also think we should remove the LOG(ERROR) lines here since it might be
confusing (it's not actually an error, and most people looking at the logs
aren't interested in this at all). Same below.

http://codereview.chromium.org/2873089/diff/69005/52024#newcode187
chrome/gpu/gpu_channel.cc:187: if (instance == NULL) return;
We would normally put the return on the following line except in very rare cases
(like maybe there are a whole bunch of them in a row). Can you change this and
also the other places here?

http://codereview.chromium.org/2873089/diff/69005/52026
File chrome/gpu/gpu_video_decoder.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52026#newcode10
chrome/gpu/gpu_video_decoder.cc:10: LOG(ERROR) <<
"GpuVideoDecoder::OnChannelConnected";
As above, I'm not a huge fan of the LOG(ERROR)s. For the ones which are
unimplemented, you can keep it if you want, but maybe add "unimplemented" to the
error message so people know what the error is. Then can you remove the ones for
the functions which do seem to basically be implemented?

http://codereview.chromium.org/2873089/diff/69005/52026#newcode39
chrome/gpu/gpu_video_decoder.cc:39: :
decoder_host_route_id_(param->decoder_host_route_id_),
Style nit, indent this 2 more spaces (and next line, too).

http://codereview.chromium.org/2873089/diff/69005/52027
File chrome/gpu/gpu_video_decoder.h (right):

http://codereview.chromium.org/2873089/diff/69005/52027#newcode42
chrome/gpu/gpu_video_decoder.h:42: private:
Order should be public, protected, private (plus merge your two protected
sections).

http://codereview.chromium.org/2873089/diff/69005/52028
File chrome/gpu/gpu_video_decoder_mft.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52028#newcode50
chrome/gpu/gpu_video_decoder_mft.cc:50: uint8* data,
Style: I'd move this to the previous line and indent the rest of the args to
match (this is preferred if everything will fit).

http://codereview.chromium.org/2873089/diff/69005/52029
File chrome/gpu/gpu_video_decoder_mft.h (right):

http://codereview.chromium.org/2873089/diff/69005/52029#newcode8
chrome/gpu/gpu_video_decoder_mft.h:8: #include "base/basictypes.h"  // For
OS_WIN.
Actually, you should use "build/build_config.h" to get the platform defines. You
will probably still need basictypes, though, but I'd put it below with the rest
of the headers.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode36
chrome/gpu/gpu_video_decoder_mft.h:36: private:
Style: normally we would have a blank line above this.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode40
chrome/gpu/gpu_video_decoder_mft.h:40: : GpuVideoDecoder(param, channel_,
handle),
Can you move the implementation of this constructor to the .cc file? As per
Elliot's recent post to chromium-dev, we want to implement constructors &
destructors in the .cc unless they really need to be inline. Otherwise, there's
a tendency for them to get inlined all over (possibly with a lot of member
constructor code) which bloats the app.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode64
chrome/gpu/gpu_video_decoder_mft.h:64: static IMFSample*
CreateInputSample(uint8* data,
Can you provide a comment for this, in particular, what the return value is and
the ownership of the pointers/return value?

http://codereview.chromium.org/2873089/diff/69005/52029#newcode70
chrome/gpu/gpu_video_decoder_mft.h:70: bool SendInput(uint8* data, int size,
int64 timestamp, int64 duration);
Can you also comment these and the return value.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode93
chrome/gpu/gpu_video_decoder_mft.h:93: };
Does it make sense to have DISALLOW_COPY_AND_ASSIGN at the bottom of this class?

http://codereview.chromium.org/2873089/diff/69005/52031
File chrome/gpu/gpu_video_service.h (right):

http://codereview.chromium.org/2873089/diff/69005/52031#newcode18
chrome/gpu/gpu_video_service.h:18: typedef struct {
I'd just do struct GpuVideoDecoderInfo { ... }; since this is C++.

Can this be moved inside the GpuVideoService (top of the private section)? It
doesn't look like it's needed outside the class.

http://codereview.chromium.org/2873089/diff/69005/52035
File chrome/renderer/gpu_video_decoder_host.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52035#newcode35
chrome/renderer/gpu_video_decoder_host.cc:35: LOG(ERROR) <<
"GpuVideoDecoderHost::Initialize";
Can you remove these?

http://codereview.chromium.org/2873089/diff/69005/52035#newcode63
chrome/renderer/gpu_video_decoder_host.cc:63: if (state_ != kStateNormal)
return;
Can you move the returns to the next line in this file?

http://codereview.chromium.org/2873089/diff/69005/52035#newcode90
chrome/renderer/gpu_video_decoder_host.cc:90: input_buffer_busy_ = false;  //
BUGBUG....
BUGBUG?

http://codereview.chromium.org/2873089/diff/69005/52035#newcode126
chrome/renderer/gpu_video_decoder_host.cc:126: input_transfer_buffer_->Unmap();
It looks like the shared memory destructor unmaps the file for you. So I think
you can replace all this code with just input_transfer_buffer_.reset(); (not
even checking for handle validity. Same for the output buffer below.

http://codereview.chromium.org/2873089/diff/69005/52035#newcode166
chrome/renderer/gpu_video_decoder_host.cc:166: int32 size =
init_param_.width_*init_param_.height_;
Style: Generally you should use spaces on each side of operators like the *
here. Same for the next few lines.

http://codereview.chromium.org/2873089/diff/69005/52036
File chrome/renderer/gpu_video_decoder_host.h (right):

http://codereview.chromium.org/2873089/diff/69005/52036#newcode43
chrome/renderer/gpu_video_decoder_host.h:43: virtual bool Initialize(const
GpuVideoDecoderInitParam& param);
For this section, it's not clear to me why these functions are virtual. Do you
expect to be deriving from this class in the future?

http://codereview.chromium.org/2873089/diff/69005/52036#newcode53
chrome/renderer/gpu_video_decoder_host.h:53: virtual ~GpuVideoDecoderHost() {}
Style nit: this should go at the top of the public section below EventHandler
(since it's a type).

[jiesun, 2010-08-05 22:41:36]

thanks for reviewing, please have another look.

address Alpha's concern:
As I said in patch description, the interface is not finalize yet. I did not
realize that hardware decodering code will exist in media/ and I do not know the
restriction of media could not depends on chrome when I writing this code. MFT
code is also not checked in when I started. we will unfork MFT code after this
patch. so all those issues listed could be addressed in parallel. how do you
think?

http://codereview.chromium.org/2873089/diff/69005/52020
File chrome/chrome_renderer.gypi (right):

http://codereview.chromium.org/2873089/diff/69005/52020#newcode21
chrome/chrome_renderer.gypi:21: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
On 2010/08/05 18:55:11, Alpha wrote:
> I don't think this is needed in the renderer.

this is explained in previous comments.

http://codereview.chromium.org/2873089/diff/69005/52021
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/2873089/diff/69005/52021#newcode235
chrome/common/gpu_messages_internal.h:235:
IPC_MESSAGE_ROUTED1(GpuVideoDecoderMsg_Initialize,
On 2010/08/05 21:22:51, brettw wrote:
> You mixed the two classes of messages (renderer->gpu and gpu->renderer) into
one
> message group. Generally we would separate these to make it more clear and
name
> them accordingly. So the messages to the host would start with
> GpuVideoDecoderHostMsg and the messages back would be named like you did the
> ones here. Can you split these apart into two IPC_BEGIN_MESSAGES(...) groups
> named appropriately?

Done.

http://codereview.chromium.org/2873089/diff/69005/52021#newcode237
chrome/common/gpu_messages_internal.h:237: // From Gpu to Render process:
confirm GpuVideoDecoder are ready or error.
On 2010/08/05 21:22:51, brettw wrote:
> Can you format these like the other messages with a blank line in between each
> one?

Done.

http://codereview.chromium.org/2873089/diff/69005/52024
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52024#newcode184
chrome/gpu/gpu_channel.cc:184: // TODO(jiesun): Add Windows 7 check.
Media Foundation is only available on vista and windows 7, since I only tested
windows 7 , I am not sure we should enable other platform.

http://codereview.chromium.org/2873089/diff/69005/52024#newcode187
chrome/gpu/gpu_channel.cc:187: if (instance == NULL) return;
On 2010/08/05 21:22:51, brettw wrote:
> We would normally put the return on the following line except in very rare
cases
> (like maybe there are a whole bunch of them in a row). Can you change this and
> also the other places here?

Done.

http://codereview.chromium.org/2873089/diff/69005/52026
File chrome/gpu/gpu_video_decoder.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52026#newcode10
chrome/gpu/gpu_video_decoder.cc:10: LOG(ERROR) <<
"GpuVideoDecoder::OnChannelConnected";
I use it as function trace, I wonder if there are already tools in chrome doing
that. but I remove it anyway

http://codereview.chromium.org/2873089/diff/69005/52026#newcode39
chrome/gpu/gpu_video_decoder.cc:39: :
decoder_host_route_id_(param->decoder_host_route_id_),
On 2010/08/05 21:22:51, brettw wrote:
> Style nit, indent this 2 more spaces (and next line, too).

Done.

http://codereview.chromium.org/2873089/diff/69005/52027
File chrome/gpu/gpu_video_decoder.h (right):

http://codereview.chromium.org/2873089/diff/69005/52027#newcode42
chrome/gpu/gpu_video_decoder.h:42: private:
On 2010/08/05 21:22:51, brettw wrote:
> Order should be public, protected, private (plus merge your two protected
> sections).

Done.

http://codereview.chromium.org/2873089/diff/69005/52028
File chrome/gpu/gpu_video_decoder_mft.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52028#newcode50
chrome/gpu/gpu_video_decoder_mft.cc:50: uint8* data,
On 2010/08/05 21:22:51, brettw wrote:
> Style: I'd move this to the previous line and indent the rest of the args to
> match (this is preferred if everything will fit).

Done.

http://codereview.chromium.org/2873089/diff/69005/52029
File chrome/gpu/gpu_video_decoder_mft.h (right):

http://codereview.chromium.org/2873089/diff/69005/52029#newcode8
chrome/gpu/gpu_video_decoder_mft.h:8: #include "base/basictypes.h"  // For
OS_WIN.
On 2010/08/05 21:22:51, brettw wrote:
> Actually, you should use "build/build_config.h" to get the platform defines.
You
> will probably still need basictypes, though, but I'd put it below with the
rest
> of the headers.

Done.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode36
chrome/gpu/gpu_video_decoder_mft.h:36: private:
On 2010/08/05 21:22:51, brettw wrote:
> Style: normally we would have a blank line above this.

Done.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode40
chrome/gpu/gpu_video_decoder_mft.h:40: : GpuVideoDecoder(param, channel_,
handle),
On 2010/08/05 21:22:51, brettw wrote:
> Can you move the implementation of this constructor to the .cc file? As per
> Elliot's recent post to chromium-dev, we want to implement constructors &
> destructors in the .cc unless they really need to be inline. Otherwise,
there's
> a tendency for them to get inlined all over (possibly with a lot of member
> constructor code) which bloats the app.

Done.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode64
chrome/gpu/gpu_video_decoder_mft.h:64: static IMFSample*
CreateInputSample(uint8* data,
On 2010/08/05 21:22:51, brettw wrote:
> Can you provide a comment for this, in particular, what the return value is
and
> the ownership of the pointers/return value?

Done.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode70
chrome/gpu/gpu_video_decoder_mft.h:70: bool SendInput(uint8* data, int size,
int64 timestamp, int64 duration);
On 2010/08/05 21:22:51, brettw wrote:
> Can you also comment these and the return value.

Done.

http://codereview.chromium.org/2873089/diff/69005/52029#newcode93
chrome/gpu/gpu_video_decoder_mft.h:93: };
On 2010/08/05 21:22:51, brettw wrote:
> Does it make sense to have DISALLOW_COPY_AND_ASSIGN at the bottom of this
class?

Done.

http://codereview.chromium.org/2873089/diff/69005/52031
File chrome/gpu/gpu_video_service.h (right):

http://codereview.chromium.org/2873089/diff/69005/52031#newcode18
chrome/gpu/gpu_video_service.h:18: typedef struct {
On 2010/08/05 21:22:51, brettw wrote:
> I'd just do struct GpuVideoDecoderInfo { ... }; since this is C++.
> 
> Can this be moved inside the GpuVideoService (top of the private section)? It
> doesn't look like it's needed outside the class.

Done.

http://codereview.chromium.org/2873089/diff/69005/52035
File chrome/renderer/gpu_video_decoder_host.cc (right):

http://codereview.chromium.org/2873089/diff/69005/52035#newcode35
chrome/renderer/gpu_video_decoder_host.cc:35: LOG(ERROR) <<
"GpuVideoDecoderHost::Initialize";
On 2010/08/05 21:22:51, brettw wrote:
> Can you remove these?

Done.

http://codereview.chromium.org/2873089/diff/69005/52035#newcode63
chrome/renderer/gpu_video_decoder_host.cc:63: if (state_ != kStateNormal)
return;
On 2010/08/05 21:22:51, brettw wrote:
> Can you move the returns to the next line in this file?

Done.

http://codereview.chromium.org/2873089/diff/69005/52035#newcode90
chrome/renderer/gpu_video_decoder_host.cc:90: input_buffer_busy_ = false;  //
BUGBUG....
On 2010/08/05 21:22:51, brettw wrote:
> BUGBUG?

Done.

http://codereview.chromium.org/2873089/diff/69005/52035#newcode126
chrome/renderer/gpu_video_decoder_host.cc:126: input_transfer_buffer_->Unmap();
On 2010/08/05 21:22:51, brettw wrote:
> It looks like the shared memory destructor unmaps the file for you. So I think
> you can replace all this code with just input_transfer_buffer_.reset(); (not
> even checking for handle validity. Same for the output buffer below.

Done.

http://codereview.chromium.org/2873089/diff/69005/52035#newcode166
chrome/renderer/gpu_video_decoder_host.cc:166: int32 size =
init_param_.width_*init_param_.height_;
On 2010/08/05 21:22:51, brettw wrote:
> Style: Generally you should use spaces on each side of operators like the *
> here. Same for the next few lines.

Done.

http://codereview.chromium.org/2873089/diff/69005/52036
File chrome/renderer/gpu_video_decoder_host.h (right):

http://codereview.chromium.org/2873089/diff/69005/52036#newcode43
chrome/renderer/gpu_video_decoder_host.h:43: virtual bool Initialize(const
GpuVideoDecoderInitParam& param);
good catch, I guesss I just copy pasted from some where.

http://codereview.chromium.org/2873089/diff/69005/52036#newcode53
chrome/renderer/gpu_video_decoder_host.h:53: virtual ~GpuVideoDecoderHost() {}
On 2010/08/05 21:22:51, brettw wrote:
> Style nit: this should go at the top of the public section below EventHandler
> (since it's a type).

Done.

[Alpha Left Google, 2010-08-05 22:43:56]

Sounds good to me. LGTM from my side.

[brettw, 2010-08-05 23:17:46]

LGTM, I think we can continue to work on this in subsequent passes.

http://codereview.chromium.org/2873089/diff/75001/45048
File chrome/gpu/gpu_video_service.h (right):

http://codereview.chromium.org/2873089/diff/75001/45048#newcode33
chrome/gpu/gpu_video_service.h:33: struct GpuVideoDecoderInfo {
Nit: indented 1 too many spaces.