<video> decode in hardware!

LANDED AS 114183 FROM http://codereview.chromium.org/8922010

<video> decode in hardware!

This uses the GpuVideoDecodeAccelerator machinery (already written to enable
ppapi to take advantage of OpenMAX HW where available) to decode <video> data.
This increases idle CPU from 20% to 45% on one particularly large (internal)
test video (red0.mp4), on an ARM crosbook.

HW decode is done on a best-effort basis; if the GPU code doesn't know how to
deal with a codec/profile we still fall back to ffmpeg for decode.  Because the
vast majority of chrome installs will be on HW with no video decode support
(yet) we only attempt HW video decode on platforms we know have a shot at it.

BUG=104579
TEST=manual testing w/ video test matrix, trybots.

[Ami GONE FROM CHROMIUM, 2011-11-30 00:08:36]

piman: please OWNERS-review for content/renderer & ppapi/
scherkus: please review everything.

This may seem like a scary CL, but it's actually quite straightforward.  The
bulk of the change is in gpu_video_decoder.{h,cc} and render_view_impl.cc. 
Almost everything else is just mechanical edits.
I made a few in-line comments (see below) as FYIs for reviewers.

Thanks!

http://codereview.chromium.org/8686010/diff/1/chrome/browser/chrome_browser_m...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8686010/diff/1/chrome/browser/chrome_browser_m...
chrome/browser/chrome_browser_main.cc:1604: if (!parameters().ui_task && false)
Ignore this - not for submission.

http://codereview.chromium.org/8686010/diff/1/content/common/gpu/media/gpu_vi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/8686010/diff/1/content/common/gpu/media/gpu_vi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:147:
gpu::gles2::TextureManager::TextureInfo* info =
The changes in this file are pulled out to
http://codereview.chromium.org/8702017/ which will land before this CL.

http://codereview.chromium.org/8686010/diff/1/content/common/gpu/media/omx_vi...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/8686010/diff/1/content/common/gpu/media/omx_vi...
content/common/gpu/media/omx_video_decode_accelerator.cc:773: DCHECK_EQ(erased,
1U);
This is fixing an actual bug that was introduced during a massive CHECK->DCHECK
replacement (that missed the fact that the body of the DCHECK_EQ had
side-effects).

http://codereview.chromium.org/8686010/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8686010/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:1907: // Glue code to expose functionality
needed by media::GpuVideoDecoder.
This could as well have lived in a separate file, but since nobody else uses it
it felt like overkill to have a separate .h/.cc pair for it.

[piman, 2011-11-30 00:48:17]

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1932: gles2_->TexParameteri(GL_TEXTURE_2D,
GL_TEXTURE_MIN_FILTER, GL_NEAREST);
GL_LINEAR for bili scaling?
Same for MAG_FILTER

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1939: return gles2_->GetError() ==
GL_NO_ERROR;
I wouldn't check the error, except maybe in debug. It causes a blocking round
trip to the gpu process.

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1944: return gles2_->GetError() ==
GL_NO_ERROR;
I don't think we need to check for error (maybe only in debug?)

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1992: new
GpuVideoDecoderFactories(gpu_channel_host, gles2, route_id)));
So, what about relative lifetimes of GpuVideoDecoderFactories vs
gpu_channel_host and gles2?
In particular, what happens if we lose the context / the GPU process crashes?
I think you need:
- keep a ref on gpu_channel_host
- don't get the gles2, but instead get a weak ref to the CommandBufferProxy, and
get the gles2 when needed (after checking for NULL
- hook up to the command buffer lost context callback chain, at some level,
maybe CommandBufferProxy (it already has a callback, set by the
WebGraphicsContext3DCommandBufferImpl, they need to play nice with each other).

Also keep in mind that the route_id above is scoped by the GpuChannelHost. We
may want to check that the CommandBufferProxy comes from the GpuChannelHost
you're getting (I believe the logic in webkit ensures that but I'm not 100%
positive it covers all possible cases...).

[scherkus (not reviewing), 2011-12-06 00:27:44]

http://codereview.chromium.org/8686010/diff/12001/content/common/gpu/media/om...
File content/common/gpu/media/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/8686010/diff/12001/content/common/gpu/media/om...
content/common/gpu/media/omx_video_decode_accelerator_unittest.cc:1002: //
TODO(fischman, vrk): add more tests!  In particular:
!!!!!!

http://codereview.chromium.org/8686010/diff/12001/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/composite_filter...
media/base/composite_filter.cc:72: bool
CompositeFilter::RemoveFilter(scoped_refptr<Filter> filter) {
unittest?

http://codereview.chromium.org/8686010/diff/12001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/filters.h#newcode86
media/base/filters.h:86: virtual void clear_host();
I couldn't find where this gets used -- also gpu_video_decoder.cc doesn't seem
to be checking for a NULL host

http://codereview.chromium.org/8686010/diff/12001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:694: state_ = kInitAudioRenderer;
does this fall back to software or is that handled by something else?

http://codereview.chromium.org/8686010/diff/12001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1149: bool ret =
pipeline_init_state_->composite_->RemoveFilter(filter.get());
can we CHECK() inside of methods like RemoveFilter if you pass in a filter that
doesn't exist and do away with frivolous bools?

http://codereview.chromium.org/8686010/diff/12001/media/base/video_decoder_co...
File media/base/video_decoder_config.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/video_decoder_co...
media/base/video_decoder_config.h:36: // Video stream profile.  This *must*
match PP_VideoDecoder_Profile.
I can't recall but I thought we didn't want to have an implicit dependency on
PPAPI code

is this compile asserted anywhere? would a conversion function be of more use if
these enums aren't used in performance critical code?

http://codereview.chromium.org/8686010/diff/12001/media/base/video_decoder_co...
media/base/video_decoder_config.h:41: VIDEO_CODEC_PROFILE_UNKNOWN = -1,
we're mixing up kStyle and ENUM_STYLE

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.cc#n...
media/base/video_frame.cc:199: return false;
DCHECK?

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.h#ne...
media/base/video_frame.h:29: // Keep in sync with WebKit::WebVideoFrame!
arg!

http://codereview.chromium.org/8686010/diff/12001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:132: case FF_PROFILE_H264_BASELINE: return
H264PROFILE_BASELINE;
let's drop the returns to the next line

http://codereview.chromium.org/8686010/diff/12001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:239: codec_context->pix_fmt =
VideoFormatToPixelFormat(config.format());
set profile here?

http://codereview.chromium.org/8686010/diff/12001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (left):

http://codereview.chromium.org/8686010/diff/12001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:40:
host()->SetError(PIPELINE_ERROR_DECODE);
\o/

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:212: next_picture_buffer_id_++, size,
texture_ids[i]));
does this method get called more than once?

do we need picture buffers w/ unique ids for the entire lifetime?

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:408: 
remove extra line

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:41: // Allocate & delete native textures. 
Return true on success.
space out methods

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:51: explicit GpuVideoDecoder(MessageLoop*
message_loop,
not explicit

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:80: private:
add 1 space

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:97: typedef std::pair<base::SharedMemory*,
size_t> SHMBuffer;
struct SHMBuffer {
  base::SharedMemory* shm;
  size_t size;
};

Avoids code like:
it->second.first->first->Close();

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:102: // Return a shared-memory segment to the
available pool.
nit: blank lines before // comments

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:105: PtsStream pts_stream_;
I'd like to get rid of this class -- did you find out a way to avoid using it?

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:120: // Populated during Initialize() (on
success) and unchanged thereafter.
nit: blank lines before // comments

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:142: std::map<int32, PictureBuffer>
picture_buffers_in_decoder_;
sanity check: do we need maps or if the count is low enough (isn't it like 3?)
would a vector of structs produce clearer code?

struct BufferPair {
  SHMBuffer* shmbuf;
  scoped_refptr<Buffer> buf;
  int32 id;
}
std::vector<BufferPair> buffers_in_decoder_;

mostly wondering if we can do away w/ all the first/second accessors code and
iterator soup if we know that we're dealing with at most 3 elements

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:143: // picture_buffer_id and the frame
wrapping the corresponding Picture, for
nit: blank lines before // comments

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:149: 
remove extra line

http://codereview.chromium.org/8686010/diff/12001/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:28: typedef VideoCodecProfile Profile;
why not use VCP?

http://codereview.chromium.org/8686010/diff/12001/webkit/media/webvideoframe_...
File webkit/media/webvideoframe_impl.h (right):

http://codereview.chromium.org/8686010/diff/12001/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:22: virtual WebVideoFrame::Format format()
const OVERRIDE;
re: discussion on chromium-dev I think we actually want to remove OVERRIDE from
these methods so we don't break the upstream WebKit builds when interfaces
change

http://codereview.chromium.org/8686010/diff/12001/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:28: virtual unsigned textureId() const
OVERRIDE;
instead of stuffing two image types (hardware and software surface) into a
single class I've contemplated having a separate interface that's dedicated for
hardware decode+render

does the hw rendering path require anything other than texture id?

I'd like to see you dig into the VideoLayerChromium code to see what we can
tease out into HW-decoded frames versus SW-decoded frames and what impact that
has on the WebVideoFrame and WebMediaPlayer interfaces

[Ami GONE FROM CHROMIUM, 2011-12-06 22:10:32]

Addressed only piman's comments in patchset 6.
scherkus@'s comments are up next!

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1932: gles2_->TexParameteri(GL_TEXTURE_2D,
GL_TEXTURE_MIN_FILTER, GL_NEAREST);
On 2011/11/30 00:48:17, piman wrote:
> GL_LINEAR for bili scaling?
> Same for MAG_FILTER

Done, although note I cargo-culted all this GL code from gles2 (where it was
cargo-culted from the old non-working GL impl in media, where it was
cargo-culted from who-knows-where).  I hope *you* know what's going on here...

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1939: return gles2_->GetError() ==
GL_NO_ERROR;
On 2011/11/30 00:48:17, piman wrote:
> I wouldn't check the error, except maybe in debug. It causes a blocking round
> trip to the gpu process.

My reasoning was that texture-creation was a rare enough event that
round-tripping was ok.
I assume you're advocating for s/bool/void/ in the return type (here and below)
and instead DCHECKing no error, and implemented that.

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1944: return gles2_->GetError() ==
GL_NO_ERROR;
On 2011/11/30 00:48:17, piman wrote:
> I don't think we need to check for error (maybe only in debug?)

Ditto.

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1992: new
GpuVideoDecoderFactories(gpu_channel_host, gles2, route_id)));
On 2011/11/30 00:48:17, piman wrote:
> So, what about relative lifetimes of GpuVideoDecoderFactories vs
> gpu_channel_host and gles2?
> In particular, what happens if we lose the context / the GPU process crashes?
> I think you need:
> - keep a ref on gpu_channel_host

Done (via scoped_ptr member).

> - don't get the gles2, but instead get a weak ref to the CommandBufferProxy,
and
> get the gles2 when needed (after checking for NULL

Done.

> - hook up to the command buffer lost context callback chain, at some level,
> maybe CommandBufferProxy (it already has a callback, set by the
> WebGraphicsContext3DCommandBufferImpl, they need to play nice with each
other).

I think I get this for (almost) free b/c a lost context will turn into a
GpuVideoDecodeAcceleratorHost::ChannelError, which I now have call
client_->NotifyError().

> Also keep in mind that the route_id above is scoped by the GpuChannelHost. 

By moving to WP<CBP>, I was able to drop route_id from the ctor arg list.

[Ami GONE FROM CHROMIUM, 2011-12-06 22:14:53]

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8686010/diff/4001/content/renderer/render_view...
content/renderer/render_view_impl.cc:1939: return gles2_->GetError() ==
GL_NO_ERROR;
On 2011/12/06 22:10:32, Ami Fischman wrote:
> I assume you're advocating for s/bool/void/ in the return type (here and
below)
> and instead DCHECKing no error, and implemented that.


Oops!  I forgot to update this after making further changes needed for
accounting for context-loss.  Return type is back to bool.

[piman, 2011-12-06 23:06:15]

OK, LGTM for that part. If you haven't already, I'd strongly suggest trying the
context lost part, by e.g. killing the GPU process.

[Ami GONE FROM CHROMIUM, 2011-12-07 00:03:04]

Responded to all of scherkus@'s CR comments (on patchset 5) in patchset 7.

http://codereview.chromium.org/8686010/diff/12001/content/common/gpu/media/om...
File content/common/gpu/media/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/8686010/diff/12001/content/common/gpu/media/om...
content/common/gpu/media/omx_video_decode_accelerator_unittest.cc:1002: //
TODO(fischman, vrk): add more tests!  In particular:
On 2011/12/06 00:27:44, scherkus wrote:
> !!!!!!

Seriously?  Have you /seen/ the amount of tests already present?

http://codereview.chromium.org/8686010/diff/12001/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/composite_filter...
media/base/composite_filter.cc:72: bool
CompositeFilter::RemoveFilter(scoped_refptr<Filter> filter) {
On 2011/12/06 00:27:44, scherkus wrote:
> unittest?

Really?  Ok, done.

http://codereview.chromium.org/8686010/diff/12001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/filters.h#newcode86
media/base/filters.h:86: virtual void clear_host();
On 2011/12/06 00:27:44, scherkus wrote:
> I couldn't find where this gets used

It's called in CompositeFilter::RemoveFilter().

> -- also gpu_video_decoder.cc doesn't seem
> to be checking for a NULL host

Done.

http://codereview.chromium.org/8686010/diff/12001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:694: state_ = kInitAudioRenderer;
On 2011/12/06 00:27:44, scherkus wrote:
> does this fall back to software or is that handled by something else?

The former.

http://codereview.chromium.org/8686010/diff/12001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1149: bool ret =
pipeline_init_state_->composite_->RemoveFilter(filter.get());
On 2011/12/06 00:27:44, scherkus wrote:
> can we CHECK() inside of methods like RemoveFilter if you pass in a filter
that
> doesn't exist and do away with frivolous bools?

Hell yes we can!

(bonus points for the use of "frivolous"!)

http://codereview.chromium.org/8686010/diff/12001/media/base/video_decoder_co...
File media/base/video_decoder_config.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/video_decoder_co...
media/base/video_decoder_config.h:36: // Video stream profile.  This *must*
match PP_VideoDecoder_Profile.
On 2011/12/06 00:27:44, scherkus wrote:
> I can't recall but I thought we didn't want to have an implicit dependency on
> PPAPI code

The dependency we couldn't take was an explicit one (which is why we have this
copy of the enum in both places instead of simply referencing one from the
other).  The implicit dep is where it's at.
(note this is a move, not a new enum)

> is this compile asserted anywhere?

No, but we have a TODO!
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/webkit/plugins/ppapi/...

> would a conversion function be of more use if
> these enums aren't used in performance critical code?

I think all the code that uses this needs to be able to handle unknown values
anyway, so not sure what a conv function buys us.

http://codereview.chromium.org/8686010/diff/12001/media/base/video_decoder_co...
media/base/video_decoder_config.h:41: VIDEO_CODEC_PROFILE_UNKNOWN = -1,
On 2011/12/06 00:27:44, scherkus wrote:
> we're mixing up kStyle and ENUM_STYLE

Yeah, we are.  Style guide prefers kStyle (newer) but doesn't justify mass
conversions.  This move highlights the inconsistency in our codebase.  Argl.

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.cc#n...
media/base/video_frame.cc:199: return false;
On 2011/12/06 00:27:44, scherkus wrote:
> DCHECK?

Done.

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/base/video_frame.h#ne...
media/base/video_frame.h:29: // Keep in sync with WebKit::WebVideoFrame!
On 2011/12/06 00:27:44, scherkus wrote:
> arg!

Done!

http://codereview.chromium.org/8686010/diff/12001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:132: case FF_PROFILE_H264_BASELINE: return
H264PROFILE_BASELINE;
On 2011/12/06 00:27:44, scherkus wrote:
> let's drop the returns to the next line

What, you think vertical space grows on trees???
Done.

http://codereview.chromium.org/8686010/diff/12001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:239: codec_context->pix_fmt =
VideoFormatToPixelFormat(config.format());
On 2011/12/06 00:27:44, scherkus wrote:
> set profile here?

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (left):

http://codereview.chromium.org/8686010/diff/12001/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:40:
host()->SetError(PIPELINE_ERROR_DECODE);
On 2011/12/06 00:27:44, scherkus wrote:
> \o/

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:212: next_picture_buffer_id_++, size,
texture_ids[i]));
On 2011/12/06 00:27:44, scherkus wrote:
> does this method get called more than once?

Yes.
Realistically: if video frame size changes, the decoder will DismissPB and
re-call PPB.
Theoretically: a decoder could ask for PB's one at a time instead of buffering
them all up.

> do we need picture buffers w/ unique ids for the entire lifetime?

I'm not sure what you're asking.  Entire lifetime of what?  The decoder?  Yes :)

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:408: 
On 2011/12/06 00:27:44, scherkus wrote:
> remove extra line

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:41: // Allocate & delete native textures. 
Return true on success.
On 2011/12/06 00:27:44, scherkus wrote:
> space out methods

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:51: explicit GpuVideoDecoder(MessageLoop*
message_loop,
On 2011/12/06 00:27:44, scherkus wrote:
> not explicit

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:80: private:
On 2011/12/06 00:27:44, scherkus wrote:
> add 1 space

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:97: typedef std::pair<base::SharedMemory*,
size_t> SHMBuffer;
On 2011/12/06 00:27:44, scherkus wrote:
> struct SHMBuffer {
>   base::SharedMemory* shm;
>   size_t size;
> };
> 
> Avoids code like:
> it->second.first->first->Close();

Meh.  I honestly don't think the replacement is much sexier, but done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:102: // Return a shared-memory segment to the
available pool.
On 2011/12/06 00:27:44, scherkus wrote:
> nit: blank lines before // comments

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:105: PtsStream pts_stream_;
On 2011/12/06 00:27:44, scherkus wrote:
> I'd like to get rid of this class -- did you find out a way to avoid using it?

Not really, although the way I use it is pretty much as a heap.  I think
pts_{stream,heap}.{h,cc} could probably be collapsed a lot...

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:120: // Populated during Initialize() (on
success) and unchanged thereafter.
On 2011/12/06 00:27:44, scherkus wrote:
> nit: blank lines before // comments

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:142: std::map<int32, PictureBuffer>
picture_buffers_in_decoder_;
On 2011/12/06 00:27:44, scherkus wrote:
> sanity check: do we need maps or if the count is low enough (isn't it like 3?)
> would a vector of structs produce clearer code?
> 
> struct BufferPair {
>   SHMBuffer* shmbuf;
>   scoped_refptr<Buffer> buf;
>   int32 id;
> }
> std::vector<BufferPair> buffers_in_decoder_;
> 
> mostly wondering if we can do away w/ all the first/second accessors code and
> iterator soup if we know that we're dealing with at most 3 elements

picture_buffers_in_decoder_ is always going to have <= 4 pairs, but
bitstream_buffers_in_decoder_ can have an unlimited number of them, and keeping
symmetry between these two variables seems better to me than introducing yet
another idiom to this class.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:143: // picture_buffer_id and the frame
wrapping the corresponding Picture, for
On 2011/12/06 00:27:44, scherkus wrote:
> nit: blank lines before // comments

Done.

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:149: 
On 2011/12/06 00:27:44, scherkus wrote:
> remove extra line

Done.

http://codereview.chromium.org/8686010/diff/12001/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/8686010/diff/12001/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:28: typedef VideoCodecProfile Profile;
On 2011/12/06 00:27:44, scherkus wrote:
> why not use VCP?

A bunch of places already refer to the inner type (implicitly by inheritance) so
this felt simpler.  Follow-up cleanup, maybe.

http://codereview.chromium.org/8686010/diff/12001/webkit/media/webvideoframe_...
File webkit/media/webvideoframe_impl.h (right):

http://codereview.chromium.org/8686010/diff/12001/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:22: virtual WebVideoFrame::Format format()
const OVERRIDE;
On 2011/12/06 00:27:44, scherkus wrote:
> re: discussion on chromium-dev I think we actually want to remove OVERRIDE
from
> these methods so we don't break the upstream WebKit builds when interfaces
> change

Done.

http://codereview.chromium.org/8686010/diff/12001/webkit/media/webvideoframe_...
webkit/media/webvideoframe_impl.h:28: virtual unsigned textureId() const
OVERRIDE;
On 2011/12/06 00:27:44, scherkus wrote:
> instead of stuffing two image types (hardware and software surface) into a
> single class I've contemplated having a separate interface that's dedicated
for
> hardware decode+render
> 
> does the hw rendering path require anything other than texture id?
> 
> I'd like to see you dig into the VideoLayerChromium code to see what we can
> tease out into HW-decoded frames versus SW-decoded frames and what impact that
> has on the WebVideoFrame and WebMediaPlayer interfaces

HW path requires only texture id & size info.  It would be great to have the HW
and SW paths of VideoLayerChromium separated, esp. b/c the HW path would just be
PluginLayerChromium (suitably renamed).  Unfortch this isn't possible b/c WK's
(current) architecture requires the type of FooLayerChromium to be decided on at
<video>-creation time, long before the type of the video stream is ascertained,
much less whether a HW decoder can be applied to it is known.
I'm discussing this with jamesr@ in
https://bugs.webkit.org/show_bug.cgi?id=73043#c22
If we find a better solution for the LayerChromium side, then it'll carry over
to this class as well (after all it wouldn't make much sense to feed a
FooVideoBlarg frame to a {Plugin,External}LayerChromium).  Until then I don't
think there's anything for me to do here.

[Ami GONE FROM CHROMIUM, 2011-12-07 00:13:11]

On 2011/12/06 23:06:15, piman wrote:
> OK, LGTM for that part. If you haven't already, I'd strongly suggest trying
the
> context lost part, by e.g. killing the GPU process.

Oops, I should have said: killing the gpu process makes the video area and its
controls bar grey-out, but the tab/page is still interactive.  If I re-click the
play button it starts playing again, but in SW-only mode.  Opening the video in
another tab resurrects the GPU process and playback is HW-decoded again.   I
think this is a reasonable state of affairs.

[scherkus (not reviewing), 2011-12-09 00:26:20]

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/8686010/diff/12001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:212: next_picture_buffer_id_++, size,
texture_ids[i]));
On 2011/12/07 00:03:04, Ami Fischman wrote:
> On 2011/12/06 00:27:44, scherkus wrote:
> > does this method get called more than once?
> 
> Yes.
> Realistically: if video frame size changes, the decoder will DismissPB and
> re-call PPB.
> Theoretically: a decoder could ask for PB's one at a time instead of buffering
> them all up.
> 
> > do we need picture buffers w/ unique ids for the entire lifetime?
> 
> I'm not sure what you're asking.  Entire lifetime of what?  The decoder?  Yes
:)

lifetime of an opened decoder

you answered my question though!

http://codereview.chromium.org/8686010/diff/32011/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8686010/diff/32011/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1934: class GpuVideoDecoderFactories :
public media::GpuVideoDecoder::Factories {
render_view_impl.cc is approaching 5000 lines

can we move this to a separate file?

http://codereview.chromium.org/8686010/diff/32011/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/composite_filter...
media/base/composite_filter.cc:62: return false;
nit: we should TODO / followup CL to make a similar change here

it would certainly simplify a bunch of code inside pipeline_impl.cc where we
check for bools all over the place

http://codereview.chromium.org/8686010/diff/32011/media/base/composite_filter...
media/base/composite_filter.cc:79: filters_.erase(it);
nit: doesn't filters_.erase(filters_.find(filter)) do the necessary crashing for
us or do you prefer our own CHECK() crashes?

http://codereview.chromium.org/8686010/diff/32011/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1130: void
PipelineImpl::DiscardFilter(scoped_refptr<Filter> filter) {
nit: maybe RemoveFilter() or even inline the code?

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.cc#n...
media/base/video_frame.cc:58: frame->planes_ = 0;
wait a tick

why is planes_ not derived from format_!?

you know what, I have a hunch we could remove both planes_ and planes() and have
consumers rely on format() doing The Right Thing

I've seen some code that does
DCHECK(for YV12)
DCHECK(for 3 planes, just in case the world has gone insane)

TODO/bugify?

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.h#ne...
media/base/video_frame.h:101: private:
\o/

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.h#ne...
media/base/video_frame.h:139: base::Closure texture_no_longer_needed_;
the optimization nerd inside me wonders if we could use a union for data fields
based on value of |format_| but I also really, really don't want to go down that
path at all hahaha

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:26: GpuVideoDecoder::SHMBuffer::~SHMBuffer()
{
collapse one-line-able fns to {}

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:33:
GpuVideoDecoder::BufferPair::~BufferPair() {
ditto

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:50: for (size_t i = 0; i <
available_shm_segments_.size(); ++i)
check out base/stl_util.h for stuff like this

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:113: PtsStream pts_stream_;
bug/todo-ify PtsStream/Heap cleanup?

http://codereview.chromium.org/8686010/diff/32011/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/8686010/diff/32011/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:28: typedef VideoCodecProfile Profile;
grepping around I didn't find where the references are -- didn't you already
change the stuff in omx_video_decode_accelerator.cc?

[Ami GONE FROM CHROMIUM, 2011-12-09 22:55:50]

http://codereview.chromium.org/8686010/diff/32011/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8686010/diff/32011/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1934: class GpuVideoDecoderFactories :
public media::GpuVideoDecoder::Factories {
On 2011/12/09 00:26:20, scherkus wrote:
> render_view_impl.cc is approaching 5000 lines
> 
> can we move this to a separate file?

Done.

http://codereview.chromium.org/8686010/diff/32011/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/composite_filter...
media/base/composite_filter.cc:62: return false;
On 2011/12/09 00:26:20, scherkus wrote:
> nit: we should TODO / followup CL to make a similar change here
> 
> it would certainly simplify a bunch of code inside pipeline_impl.cc where we
> check for bools all over the place

Done.

http://codereview.chromium.org/8686010/diff/32011/media/base/composite_filter...
media/base/composite_filter.cc:79: filters_.erase(it);
On 2011/12/09 00:26:20, scherkus wrote:
> nit: doesn't filters_.erase(filters_.find(filter)) do the necessary crashing
for
> us or do you prefer our own CHECK() crashes?

filters_ is a vector so no find() for you.
Could use std::find() but relying on vector to crash when given its end() to
erase is unlikely to be reliable in opt mode.

http://codereview.chromium.org/8686010/diff/32011/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:1130: void
PipelineImpl::DiscardFilter(scoped_refptr<Filter> filter) {
On 2011/12/09 00:26:20, scherkus wrote:
> nit: maybe RemoveFilter() or even inline the code?

Done.

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.cc#n...
media/base/video_frame.cc:58: frame->planes_ = 0;
On 2011/12/09 00:26:20, scherkus wrote:
> wait a tick
> 
> why is planes_ not derived from format_!?
> 
> you know what, I have a hunch we could remove both planes_ and planes() and
have
> consumers rely on format() doing The Right Thing
> 
> I've seen some code that does
> DCHECK(for YV12)
> DCHECK(for 3 planes, just in case the world has gone insane)
> 
> TODO/bugify?

http://code.google.com/p/chromium/issues/detail?id=107043

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.h#ne...
media/base/video_frame.h:101: private:
On 2011/12/09 00:26:20, scherkus wrote:
> \o/

Done.

http://codereview.chromium.org/8686010/diff/32011/media/base/video_frame.h#ne...
media/base/video_frame.h:139: base::Closure texture_no_longer_needed_;
On 2011/12/09 00:26:20, scherkus wrote:
> the optimization nerd inside me wonders if we could use a union for data
fields
> based on value of |format_| but I also really, really don't want to go down
that
> path at all hahaha

Done.

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:26: GpuVideoDecoder::SHMBuffer::~SHMBuffer()
{
On 2011/12/09 00:26:20, scherkus wrote:
> collapse one-line-able fns to {}

Done.

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:33:
GpuVideoDecoder::BufferPair::~BufferPair() {
On 2011/12/09 00:26:20, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:50: for (size_t i = 0; i <
available_shm_segments_.size(); ++i)
On 2011/12/09 00:26:20, scherkus wrote:
> check out base/stl_util.h for stuff like this

Done.

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/8686010/diff/32011/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:113: PtsStream pts_stream_;
On 2011/12/09 00:26:20, scherkus wrote:
> bug/todo-ify PtsStream/Heap cleanup?

http://code.google.com/p/chromium/issues/detail?id=107036

http://codereview.chromium.org/8686010/diff/32011/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/8686010/diff/32011/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:28: typedef VideoCodecProfile Profile;
On 2011/12/09 00:26:20, scherkus wrote:
> grepping around I didn't find where the references are -- didn't you already
> change the stuff in omx_video_decode_accelerator.cc?

No; it's trickier; other mentions include: GVDA, GVDAH, gpu_messages (all easy).
 But then you run into ppapi impl and that's a whole 'nother mess of OWNERS and
I wanted to avoid that in this CL.
TODO'd instead.

[scherkus (not reviewing), 2011-12-09 23:13:55]

lgtm w/ pedantic nits

http://codereview.chromium.org/8686010/diff/46002/content/renderer/renderer_g...
File content/renderer/renderer_gpu_video_decoder_factories.h (right):

http://codereview.chromium.org/8686010/diff/46002/content/renderer/renderer_g...
content/renderer/renderer_gpu_video_decoder_factories.h:22: class
RendererGpuVideoDecoderFactories :
9 of 10 header files in chrome agree that colons go on the next line

http://codereview.chromium.org/8686010/diff/46002/content/renderer/renderer_g...
content/renderer/renderer_gpu_video_decoder_factories.h:25: virtual
~RendererGpuVideoDecoderFactories();
don't we typically put dtors after ctors?

http://codereview.chromium.org/8686010/diff/46002/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/8686010/diff/46002/media/base/composite_filter...
media/base/composite_filter.cc:60: // TODO(fischman,scherkus): s/bool/void/ the
return type and CHECK on failure
woah woah woah!

[Ami GONE FROM CHROMIUM, 2011-12-10 00:03:14]

Thanks for the review!

http://codereview.chromium.org/8686010/diff/46002/content/renderer/renderer_g...
File content/renderer/renderer_gpu_video_decoder_factories.h (right):

http://codereview.chromium.org/8686010/diff/46002/content/renderer/renderer_g...
content/renderer/renderer_gpu_video_decoder_factories.h:25: virtual
~RendererGpuVideoDecoderFactories();
On 2011/12/09 23:13:55, scherkus wrote:
> don't we typically put dtors after ctors?

Chrome does (which is suboptimal in general but fine given other constraints
enforced by chrome style).  Done.

http://codereview.chromium.org/8686010/diff/46002/media/base/composite_filter.cc
File media/base/composite_filter.cc (right):

http://codereview.chromium.org/8686010/diff/46002/media/base/composite_filter...
media/base/composite_filter.cc:60: // TODO(fischman,scherkus): s/bool/void/ the
return type and CHECK on failure
On 2011/12/09 23:13:55, scherkus wrote:
> woah woah woah!

Whaaaaat?  You asked for a TODO, you got a TODO.

[commit-bot: I haz the power, 2011-12-10 00:04:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/8686010/49089

[commit-bot: I haz the power, 2011-12-10 00:04:36]

Can't process patch for file media/base/composite_filter.h.
File's status is None, patchset upload is incomplete.

[Ami GONE FROM CHROMIUM, 2011-12-10 01:24:44]

CQ was busted (b/c of rietveld missing file status for several files in the CL
despite multiple uploads).  Committed manually as 113895 but then reverted due
to: missing virtual keyword (broke clang compile), missing MEDIA_EXPORT on
GpuVideoDecoder::Factories (broke win dbg shared compile), and missing
MEDIA_EXPORT on RunPipelineStatusOKCB (broke linux shared compile).  Reverted in
113908.  Will upload new patchset and send to lots more trybots.

[Ami GONE FROM CHROMIUM, 2011-12-12 19:13:48]

I think I pacified all the bots now (piecemeal; am now kicking off new tryjobs
for all of them).  

piman: PTAL?  (scherkus is teaching a class all day).  Thanks!

[Ami GONE FROM CHROMIUM, 2011-12-12 19:21:28]

> piman: PTAL?  (scherkus is teaching a class all day).  Thanks!

In fact in order to enable landing with the CQ, I've uploaded to a new issue:
http://codereview.chromium.org/8922010, where PS#1 is PS#13 here.  Once you're
satisfied with the diffs here please LGTM there.

[scherkus (not reviewing), 2011-12-13 00:36:47]

LGTM += 2

http://codereview.chromium.org/8686010/diff/64001/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/8686010/diff/64001/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:35: class MEDIA_EXPORT Factories {
craziness