Add VAVDA, the VAAPI Video Decode Accelerator for Intel CPUs. (issue 9814001)

104 views

Skip to first unread message

pos...@chromium.org

unread,

Mar 21, 2012, 2:57:24 AM3/21/12

to fisc...@chromium.org, chromium...@chromium.org, dhollow...@chromium.org, j...@chromium.org, apat...@chromium.org, joi+watc...@chromium.org, feature-me...@chromium.org, dari...@chromium.org

Reviewers: Ami Fischman,

Description:
Add VAVDA, the VAAPI Video Decode Accelerator for Intel CPUs.

VAVDA is the hardware video decode accelerator for Chrome on Linux
and ChromeOS for Intel CPUs (Sandy Bridge+).

This CL enables VAVDA acceleration for ChromeOS, both for HTML5 video
and Flash.

This is an initial version of the decoder and requires the stream parser
(see separate "H264 parser for VAVDA" CL). Note that video will
be displayed upside-down, due to assumptions currently being made in the
renderer and it is not a bug in VAVDA. This will be fixed by a separate CL.
Some reset stability issues and random stream seek still have
to be ironed out.

The feature is currently hidden behind a command line flag and can be
enabled by adding the --enable-vaapi-decode-accel parameter to command
line.

Add a H.264 parser for VAVDA.

VAVDA is the hardware video decode accelerator for Chrome on Linux
and ChromeOS for Intel CPUs (Sandy Bridge+).

This is the first part of a series, adding a bitstream parser
that will be used by VAVDA.

BUG=117062
TEST=Manual runs of test streams.

Please review this at https://chromiumcodereview.appspot.com/9814001/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M content/browser/gpu/gpu_process_host.cc
M content/common/gpu/media/gpu_video_decode_accelerator.cc
A content/common/gpu/media/h264_dpb.h
A content/common/gpu/media/h264_dpb.cc
A content/common/gpu/media/h264_parser.h
A content/common/gpu/media/h264_parser.cc
A content/common/gpu/media/h264_parser_unittest.cc
A content/common/gpu/media/vaapi_h264_decoder.h
A content/common/gpu/media/vaapi_h264_decoder.cc
A content/common/gpu/media/vaapi_video_decode_accelerator.h
A content/common/gpu/media/vaapi_video_decode_accelerator.cc
M content/content_common.gypi
M content/content_tests.gypi
M content/public/common/content_switches.h
M content/public/common/content_switches.cc
M content/renderer/render_view_impl.cc
A third_party/libva/README.chromium
A third_party/libva/va/va.h
A third_party/libva/va/va_android.h
A third_party/libva/va/va_backend.h
A third_party/libva/va/va_backend_egl.h
A third_party/libva/va/va_backend_glx.h
A third_party/libva/va/va_backend_tpi.h
A third_party/libva/va/va_dri.h
A third_party/libva/va/va_dri2.h
A third_party/libva/va/va_dricommon.h
A third_party/libva/va/va_egl.h
A third_party/libva/va/va_fool.h
A third_party/libva/va/va_glx.h
A third_party/libva/va/va_tpi.h
A third_party/libva/va/va_trace.h
A third_party/libva/va/va_version.h
A third_party/libva/va/va_x11.h
M ui/gfx/gl/gl_context_egl.h
M ui/gfx/gl/gl_context_egl.cc
M ui/gfx/gl/gl_context_glx.h
M ui/gfx/gl/gl_context_glx.cc

fisc...@chromium.org

unread,

Mar 21, 2012, 9:16:24 AM3/21/12

to pos...@chromium.org, chromium...@chromium.org, dhollow...@chromium.org, j...@chromium.org, apat...@chromium.org, joi+watc...@chromium.org, feature-me...@chromium.org, dari...@chromium.org

I ran out of code-reviewing steam at vaapi_h264_decoder.cc:1739 (and before
looking through VAVDA.{h,cc}).
I'll hold off looking at the rest until after you address previous comments.
Please make an effort to bring the whole CL into style-guide compliance, as
it
makes it much harder than it needs to be to review this code as-is.
Also, please make an effort to reduce duplication of text where possible (I
made
a couple of comments to this effect elsewhere, but wanted to call this out
specifically as something to think about at the whole-CL level.
Finally, please make a pass in rietveld to ensure code you meant to drop is
gone
(commented-out lines), and TODOs are either in proper format
("TODO(posciak):
what needs to be done and why") or are gone (i.e. no "TODO posciak think
about
this") :)

If you want me to look at something specific before addressing my other
comments
please let me know.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/browser/gpu/gpu_process_host.cc
File content/browser/gpu/gpu_process_host.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/browser/gpu/gpu_process_host.cc#newcode1
content/browser/gpu/gpu_process_host.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
I think the CL description has a bit of copy/paste leftover (search for
"bitstream")

https://chromiumcodereview.appspot.com/9814001/diff/1/content/browser/gpu/gpu_process_host.cc#newcode705
content/browser/gpu/gpu_process_host.cc:705:
switches::kEnableVaapiDecodeAcceleration,
Keep sorted?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode27
content/common/gpu/media/gpu_video_decode_accelerator.cc:27: #else //
OS_WIN
Having #else\n#if is confusing; replace with #elif?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode36
content/common/gpu/media/gpu_video_decode_accelerator.cc:36: #include
"ui/gfx/gl/gl_context.h"
dup of l.40 below?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode155
content/common/gpu/media/gpu_video_decode_accelerator.cc:155: DLOG(INFO)
<< "Initializing VAAPI HW decoder.";
drop

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode159
content/common/gpu/media/gpu_video_decode_accelerator.cc:159:
(gfx::GLContextGLX*) stub_->decoder()->GetGLContext();
Both this and the cast below should be static_cast<>'s

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc
File content/common/gpu/media/h264_dpb.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc#newcode59
content/common/gpu/media/h264_dpb.cc:59: PicsVector::iterator iter =
std::find_if(pics_.begin(), pics_.end(),
I don't think using the STL algorithm business is buying you much in
line count, and IMO it's costing quite a bit in readability. Why not
the (IMO much simpler):

void RemovePic(size_t index) {
delete pics_[index];
pics_.begin() + index;
}

for (int i = 0; i < pics_.size(); ++i) {
if (pics_[i]->PicOrderCnt != poc)
continue;
RemovePic(i);
return;
}
DVLOG(FATAL) << "Missing POC: " << poc;

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc#newcode66
content/common/gpu/media/h264_dpb.cc:66: PicsVector::iterator iter =
pics_.begin();
Ditto replace functional style w/ explicit:
for (PicsVector::iterator it = pics_.begin(); it != pics_.end(); ) {
if (*it->outputted && !*it->ref)
pics_->erase(it++);
else
++it;
}

Esp. b/c of large # of structs you end up having to define, I believe
this will be a significant readability improvement. I'm open to
discussion if you feel strongly otherwise.
I'll stop commenting about this in this CL now.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc#newcode98
content/common/gpu/media/h264_dpb.cc:98: H264Picture*
H264DPB::GetRefPicByPicNum(int pic_num, bool long_term) {
I suspect you'll want to simply drop this function (since the loop is so
straightforward there's no reason to not dup it in
Get{Short,Long}RefBy*()

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc#newcode132
content/common/gpu/media/h264_dpb.cc:132: return
*(std::min_element(short_refs.begin(), short_refs.end(),
Ow, that hurted my brain!

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode28
content/common/gpu/media/h264_dpb.h:28: int TopFieldOrderCnt;
Naming in this file is inconsistent. Please style-guide it up.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode56
content/common/gpu/media/h264_dpb.h:56: // Utility function objects
These make me sad. Are they really all needed?
Can they be defined near their only use if they're only used in one
(.cc) file?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode113
content/common/gpu/media/h264_dpb.h:113: typedef
std::vector<H264Picture*> PicsVector;
Move into H264Picture and and rename to just Vector?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode118
content/common/gpu/media/h264_dpb.h:118: class H264DPB {
Sad that the spec defines DPB as a buffer of decoded pictures, but other
VDA terminology uses PictureBuffer for a single image :(

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode133
content/common/gpu/media/h264_dpb.h:133: void StorePic(H264Picture*
pic);
If you passed by scoped_ptr you wouldn't have to document ownership
transfer.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode142
content/common/gpu/media/h264_dpb.h:142: H264Picture*
GetShortRefPicByPicNum(int pic_num);
Assuming this isn't relinquishing ownership, IWBN to return const-ref
instead of pointer here (and below).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode161
content/common/gpu/media/h264_dpb.h:161: // Iterators for direct access
to DPB contents.
These will be invalidated by mutating methods elsewhere in this class.
Do you want to document anything about that?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode174
content/common/gpu/media/h264_dpb.h:174:
extra newline

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.cc
File content/common/gpu/media/h264_parser.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.cc#newcode4
content/common/gpu/media/h264_parser.cc:4:
Not part of this CL?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.h
File content/common/gpu/media/h264_parser.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.h#newcode2
content/common/gpu/media/h264_parser.h:2: // Use of this source code is
governed by a BSD-style license that can be
Not part of this CL?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser_unittest.cc
File content/common/gpu/media/h264_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser_unittest.cc#newcode4
content/common/gpu/media/h264_parser_unittest.cc:4:
Not part of this CL?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc
File content/common/gpu/media/vaapi_h264_decoder.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode16
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_SUCCESS_OR_ERROR(vares, err_msg) \
this name is confusing
(why, yes, I'm sure we either had success or had an error ;))

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode19
content/common/gpu/media/vaapi_h264_decoder.cc:19: DLOG(ERROR) <<
(err_msg); \
s/DLOG/DVLOG/ everywhere unless you have a reason not to.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode20
content/common/gpu/media/vaapi_h264_decoder.cc:20: DLOG(ERROR) << "VA
error: " << vaapi_vaErrorStr(vares); \
Can these be a single LOG?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode29
content/common/gpu/media/vaapi_h264_decoder.cc:29: return (ret);
\
Don't most of these cases also want to set state_=kError?
Is there a scheme for deciding when to set state_ to kError and when not
to?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode37
content/common/gpu/media/vaapi_h264_decoder.cc:37: typedef VADisplay
(*vaapiGetDisplayGLX)(Display *dpy);
Types need to be Capitalized (here and elsewhere)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode38
content/common/gpu/media/vaapi_h264_decoder.cc:38:
remove the newlines between these typedefs?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode122
content/common/gpu/media/vaapi_h264_decoder.cc:122: vaapiGetDisplayGLX
vaapi_vaGetDisplayGLX =
s/vaapi_vaGetDisplayGLX/VAAPI_GetDisplayGLX/
(and similar renaming throughout)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode123
content/common/gpu/media/vaapi_h264_decoder.cc:123:
reinterpret_cast<vaapiGetDisplayGLX>(dlsym(vaapi_glx_handle,
does using a macro for this make sense?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode187
content/common/gpu/media/vaapi_h264_decoder.cc:187: return
(vaapi_vaGetDisplayGLX
drop the parens?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode254
content/common/gpu/media/vaapi_h264_decoder.cc:254:
num_available_decode_surfaces_ = num_assigned_vaapi_surfaces_;
if you s/num_assigned_vaapi_surfaces_/decode_surfaces_.size()/
then you can drop the requirement from l.208 of assigning
num_assigned_vaapi_surfaces_ before calling Reset()?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode259
content/common/gpu/media/vaapi_h264_decoder.cc:259: // Still initialized
and ready to decode
this is a lie during the call from the ctor

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode263
content/common/gpu/media/vaapi_h264_decoder.cc:263: void
VaapiH264Decoder::Destroy() {
early-return if state_==kUninitialized ?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode268
content/common/gpu/media/vaapi_h264_decoder.cc:268: switch (state_) {
Reset() just set state_; why is this switch necessary?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode284
content/common/gpu/media/vaapi_h264_decoder.cc:284: default:
better to drop default: and let the compiler yell at you if you forgot a
case.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode292
content/common/gpu/media/vaapi_h264_decoder.cc:292: return
AreVaapiFunctionPointersInitialized();
This doesn't check HW support, only presence of libraries.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode311
content/common/gpu/media/vaapi_h264_decoder.cc:311:
extra \n

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode315
content/common/gpu/media/vaapi_h264_decoder.cc:315: // Has to be run in
GLXContext thread
replace this comment (and others like it) with DCHECK at the top of the
function.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode322
content/common/gpu/media/vaapi_h264_decoder.cc:322: VAStatus va_res;
first use is at l.349; move this there.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode362
content/common/gpu/media/vaapi_h264_decoder.cc:362: DLOG(INFO) <<
"YUV420 not supported";
s/DLOG/DVLOG/
s/INFO/ERROR/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode382
content/common/gpu/media/vaapi_h264_decoder.cc:382: {}
opening brace belongs on previous line

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode394
content/common/gpu/media/vaapi_h264_decoder.cc:394: GLX_Y_INVERTED_EXT,
GL_TRUE,
what's this about?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode419
content/common/gpu/media/vaapi_h264_decoder.cc:419: static int x11_error
= 0;
Oh my.
X11 error handling is tricky tricky business. I hope this was only here
as a debugging aid for you & you were planning to drop it from the CL
before submission.

You might be interested in
http://code.google.com/codesearch#OAMlx_jo-ck/src/ui/base/x/x11_util_internal.h&type=cs&l=39

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode473
content/common/gpu/media/vaapi_h264_decoder.cc:473: XSync(x_display_,
False);
I don't think it's ok to have XSync's without documented reason (I
expect them not to be needed).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode506
content/common/gpu/media/vaapi_h264_decoder.cc:506:
scoped_ptr<DecodeSurface> dec_surface(new DecodeSurface());
s/scoped/linked/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode508
content/common/gpu/media/vaapi_h264_decoder.cc:508: DCHECK_EQ(state_,
kDecoding);
how can you be sure? What if an error occurred but was only delivered
after the AssignPictureBuffer task was queued?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode536
content/common/gpu/media/vaapi_h264_decoder.cc:536:
make_linked_ptr(dec_surface.release()))).second;
indent is wrong (and make_linked_ptr/release shouldn't be necessary
after you change dec_surface to be a linked_ptr).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode549
content/common/gpu/media/vaapi_h264_decoder.cc:549: pic_height_,
VA_RT_FORMAT_YUV420,
indentation here and elsewhere is off. please make a pass.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode567
content/common/gpu/media/vaapi_h264_decoder.cc:567: VAStatus va_status;
please standardize on a single name for this throughout the file.
I think "res" is fine (as opposed to va_res/va_status/etc).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode582
content/common/gpu/media/vaapi_h264_decoder.cc:582:
poc_to_decode_surfaces_[pic->PicOrderCnt]->va_surface_id;
operator[] will add a NULL pointer if the index is not found. Ok?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode598
content/common/gpu/media/vaapi_h264_decoder.cc:598: NOTREACHED();
drop default: clause.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode613
content/common/gpu/media/vaapi_h264_decoder.cc:613: // Return reference
frames in reverse order of insertion
what's the reverse order about?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode614
content/common/gpu/media/vaapi_h264_decoder.cc:614: for (rit =
dpb_.rbegin(), i = 0; rit != dpb_.rend() && i < 16; ++rit)
s/16/kSomething/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode615
content/common/gpu/media/vaapi_h264_decoder.cc:615: if ((*rit)->ref)
multi-line for bodies require braces.
here & elsewhere; please make a pass to make sure this is taken care of
for all control structures.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode642
content/common/gpu/media/vaapi_h264_decoder.cc:642:
poc_to_decode_surfaces_[poc] = iter->second.get();
not worried about overwriting a previous value?
(why use operator[] instead of .insert() + DCHECK ret val?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode654
content/common/gpu/media/vaapi_h264_decoder.cc:654: /*DCHECK(it !=
poc_to_decode_surfaces_.end());*/
drop?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode667
content/common/gpu/media/vaapi_h264_decoder.cc:667: va_pic->picture_id =
0xffffffff;
kuint32max?
(but why set it to this?)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode686
content/common/gpu/media/vaapi_h264_decoder.cc:686:
pic_param.picture_width_in_mbs_minus1 = sps->pic_width_in_mbs_minus1;
I'm weeping over here over the amount of text involved in this. No
better way?
For example, is it a bad idea to have the parser emit to the VA-specific
structures to avoid all this manual copying?
(and as a nice side-effect, would allow you to drop the lengthy structs
from the parser's definition).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode783
content/common/gpu/media/vaapi_h264_decoder.cc:783: for (i = 0; i < 2;
++i)
I'm surprised by the '2' here and at l.793, expecting to see 6's.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode954
content/common/gpu/media/vaapi_h264_decoder.cc:954: // TODO posciak:
start using vaMapBuffer instead of vaCreateBuffer wherever
What's your plan here?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode985
content/common/gpu/media/vaapi_h264_decoder.cc:985: buffers[i] =
pending_va_bufs_.front();
If 32 is a real limit, why not just maintain pending_va_bufs_ &
pending_slice_bufs_ as VABufferID[kMaxVABuffers] along with
pending_{va,slice}_count_, and avoid this business with having to copy
them over here?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1017
content/common/gpu/media/vaapi_h264_decoder.cc:1017: frame_decoded_ =
true;
this var name and the comment preceding it are at odds.
At this point in time has the frame been decoded or not?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1031
content/common/gpu/media/vaapi_h264_decoder.cc:1031: curr_pic_->field =
slice_hdr->bottom_field_flag ? H264Picture::FIELD_BOTTOM
multi-line then clause requires braces for then & else clauses.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1037
content/common/gpu/media/vaapi_h264_decoder.cc:1037: // Not correct if
not a field.
not sure what this means.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1057
content/common/gpu/media/vaapi_h264_decoder.cc:1057:
DCHECK_EQ(sizeof(curr_pic_->ref_pic_marking),
This is a COMPILE_ASSERT that can go somewhere (not in the decode
code-path)?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1074
content/common/gpu/media/vaapi_h264_decoder.cc:1074: switch
(parser_.GetSPS(curr_sps_id_)->pic_order_cnt_type) {
since only case 0 is handled it would be much clearer (and allow less
indentation) if you checked for the != 0 case and errored-out early.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1123
content/common/gpu/media/vaapi_h264_decoder.cc:1123: break;
break is unnecessary after return (here and elsewhere).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1139
content/common/gpu/media/vaapi_h264_decoder.cc:1139:
extra newlines are unnecessary.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1148
content/common/gpu/media/vaapi_h264_decoder.cc:1148: default:
drop default clause

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1156
content/common/gpu/media/vaapi_h264_decoder.cc:1156: H264Picture* pic;
move to first use at l.1159.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1165
content/common/gpu/media/vaapi_h264_decoder.cc:1165: // Below is not
correct if a field.
unclear

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1188
content/common/gpu/media/vaapi_h264_decoder.cc:1188:
std::sort(RefPicList0_.begin(), RefPicList0_.end(),
This (and below) is an awful lot of std::sort()'ing.
Is it really necessary?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1194
content/common/gpu/media/vaapi_h264_decoder.cc:1194:
//DCHECK((int)RefPicList0_.size() <=
drop

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1195
content/common/gpu/media/vaapi_h264_decoder.cc:1195:
//slice_hdr->num_ref_idx_l0_active_minus1 + 1);
drop

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1272
content/common/gpu/media/vaapi_h264_decoder.cc:1272: if (!pic)
when can this happen?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1291
content/common/gpu/media/vaapi_h264_decoder.cc:1291: int i;
Please declare variables at the latest possible time.
(here and elsewhere; please make a pass)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1335
content/common/gpu/media/vaapi_h264_decoder.cc:1335: -
((int)list_mod->abs_diff_pic_num_minus1 + 1)
indent is wrong here & elsewhere
operators belong on preceding line, always.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1383
content/common/gpu/media/vaapi_h264_decoder.cc:1383: // Shift all
pictures starting from refIdxLX to the right.
Can you try to extract common functionality into helper functions to
avoid duplicate structures?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1419
content/common/gpu/media/vaapi_h264_decoder.cc:1419: // Must be run in
GLXContext thread.
Replace this comment with a DCHECK at the top of the function.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1432
content/common/gpu/media/vaapi_h264_decoder.cc:1432: // Put the decoded
data into XPixmap bound to the texture.
What is this doing?
The comment reads to me like "copy from the VA surface to the pixmap"
but I thought you said this is zero-copy. What's up?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1456
content/common/gpu/media/vaapi_h264_decoder.cc:1456: // require the data
to be synced with texture contents. Client must use
Why do it this way?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1464
content/common/gpu/media/vaapi_h264_decoder.cc:1464: DCHECK(msg_loop_);
I thought you were going to PostTask to this msg loop, not DCHECK it
here.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1518
content/common/gpu/media/vaapi_h264_decoder.cc:1518: // Prepare
reference picture lists if required (B and S/SP slices).
comment is conditional but next two lines aren't.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1528
content/common/gpu/media/vaapi_h264_decoder.cc:1528:
lots of unnecessary \n's IMO

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1530
content/common/gpu/media/vaapi_h264_decoder.cc:1530: DVLOG(4) <<
"================ B slice, POC= " << curr_pic_->PicOrderCnt
I think most/all of these DVLOGs in this CL shouldn't be submitted.
There's already a ton of code here; adding a significant # of lines for
debugging output doesn't seem worthwhile to me.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1555
content/common/gpu/media/vaapi_h264_decoder.cc:1555: struct
MarkLongTermPicUnusedForRefIfOverMax
ditto my comment about explicit loops over algorithms here and below.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1722
content/common/gpu/media/vaapi_h264_decoder.cc:1722: // Shouldn't get
here.
NOTIMPLEMENTED is LOG(ERROR). Can you replace some usages of it with
DCHECKs instead?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h
File content/common/gpu/media/vaapi_h264_decoder.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode12
content/common/gpu/media/vaapi_h264_decoder.h:12: #include
"h264_parser.h"
these includes are unstylish (belong in order below, with
content/common/gpu/media/ prefix)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode21
content/common/gpu/media/vaapi_h264_decoder.h:21: #include
"base/memory/linked_ptr.h"
Any reason you didn't use linked_ptr for PicsVector?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode22
content/common/gpu/media/vaapi_h264_decoder.h:22: #include
"base/memory/ref_counted.h"
unused?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode24
content/common/gpu/media/vaapi_h264_decoder.h:24: #include
"base/message_loop.h"
fwd-declare

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode26
content/common/gpu/media/vaapi_h264_decoder.h:26:
extra \n

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode29
content/common/gpu/media/vaapi_h264_decoder.h:29: // A H264 decoder for
use for VA-API-specific decoding. Provides features not
s/A/An/ (here and elsewhere in comments in this CL)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode39
content/common/gpu/media/vaapi_h264_decoder.h:39: // using the Texture
From Pixmap GLX extension.
is this really a concern of this class?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode44
content/common/gpu/media/vaapi_h264_decoder.h:44: // is further referred
to as "GLX thread", while decoding thread is referred
Is there some reason not to refer to this as the ChildThread
(chromium-wide convention & class)?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode45
content/common/gpu/media/vaapi_h264_decoder.h:45: // to as "decoder
thread". If not specified otherwise, a function is to be
I think this is a problematic contract. Is there some reason not to
annotate every public function with "Call on ChildThread." vs. "Call on
decode thread."?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode49
content/common/gpu/media/vaapi_h264_decoder.h:49:
extra newline

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode50
content/common/gpu/media/vaapi_h264_decoder.h:50: // Callback to be
invoked on the client when a picture
sentence is cut off

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode54
content/common/gpu/media/vaapi_h264_decoder.h:54: // Client is expected
to call PutPicToTexture() to when it is ready to
Please make an editing pass on this paragraph to english it up.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode56
content/common/gpu/media/vaapi_h264_decoder.h:56: typedef void
(*OutputPicCallbackPtr)(void*, int32, int32);
Why not instead
typedef base::Callback<void(int32, int32)> PictureReadyCB;
?

By using a Callback instead of a function-pointer you get to bind state
into the callback (so you don't need the C-flavored void*) and you get
refcounting for free (i.e. to ensure that the VDA doesn't go away before
the OPCP is deref'd and called)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode60
content/common/gpu/media/vaapi_h264_decoder.h:60: kDecError, // error
while decoding
Chromium style eschews abbreviations. I'm not commenting on it in parts
of the CL that are tied to the h264 spec (where I believe you're using
spec abbreviations to make mapping between the spec and the code
easier), but please stick to chromium naming style in non-spec-specific
stuff.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode61
content/common/gpu/media/vaapi_h264_decoder.h:61: // TODO posciak: add
unsupported stream error (for now treated as error in
can this be done in this CL?
(if not, can you remove this TODO & commented-out enum value, or else
expand the TODO with what your plan is?)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode63
content/common/gpu/media/vaapi_h264_decoder.h:63: //KStreamError, //
error in stream
s/KS/kS/
(also missing space before k)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode74
content/common/gpu/media/vaapi_h264_decoder.h:74: // |msg_loop| is where
the |output_pic_callback| should be posted, with
Is msg_loop the decoder thread or the child thread?
If the latter, then why not use MessageLoop::current() in the impl of
Initialize (given you require Initialize must be called on the child
thread)?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode85
content/common/gpu/media/vaapi_h264_decoder.h:85: // TODO reorganize
private/public
Yes, please. (it's hard to review this class without being sure what
its API is)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode99
content/common/gpu/media/vaapi_h264_decoder.h:99: bool
PutPicToTexture(int32 picture_buffer_id);
See comment elsewhere, but hopefully this can be inlined into
OutputPic() or whatever ends up calling the picture-ready callback.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode103
content/common/gpu/media/vaapi_h264_decoder.h:103: bool Flush();
what's the ret value?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode107
content/common/gpu/media/vaapi_h264_decoder.h:107: // state, so the
playback of the same stream can be resumed (possibly from
What state needs to be kept across Reset()? I am surprised that any
state is useful at that point.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode119
content/common/gpu/media/vaapi_h264_decoder.h:119: DecResult
DecodeInitial(int32 input_id);
Any reason not to make this part of SetStream?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode127
content/common/gpu/media/vaapi_h264_decoder.h:127: // Valid only after a
successful Initialize().
s/Initialize/DecodeInitial/ ?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode128
content/common/gpu/media/vaapi_h264_decoder.h:128: int pic_height() {
return pic_height_; }
s/pic_// here and below.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode136
content/common/gpu/media/vaapi_h264_decoder.h:136:
extra \n

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode142
content/common/gpu/media/vaapi_h264_decoder.h:142: enum {
kNumReqPictures = H264DPB::kDPBMaxSize + 6 };
s/6/1 + media::limits::kMaxVideoFrames/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode150
content/common/gpu/media/vaapi_h264_decoder.h:150: kError, // error in
kDecoding state
initial caps above please

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode221
content/common/gpu/media/vaapi_h264_decoder.h:221:
scoped_ptr<H264Picture> curr_pic_;
Why not a simple member?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode237
content/common/gpu/media/vaapi_h264_decoder.h:237: // Values related to
previously decoded reference picture.
Does it not make sense to have a
H264Picture last_ref_picture_;
?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode251
content/common/gpu/media/vaapi_h264_decoder.h:251:
extra \n

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode253
content/common/gpu/media/vaapi_h264_decoder.h:253: // Vaapi-related
functions.
style guide requires functions before members.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode261
content/common/gpu/media/vaapi_h264_decoder.h:261: // These queue up
data for HW decoder to be commited on running HW decode.
committed

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode284
content/common/gpu/media/vaapi_h264_decoder.h:284: struct DecodeSurface
{
There is private state and methods in this, so it's a class, not a
struct (and fields need to be renamed & set w/ setters and read with
getters). But I think most of the fields are read-only and could be set
in the ctor.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode303
content/common/gpu/media/vaapi_h264_decoder.h:303: DecodeSurface();
move these to the top of the class

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode308
content/common/gpu/media/vaapi_h264_decoder.h:308: bool
BindToTexture(int width, int height);
inline into ctor and replace bool ret value with setting available to
false, which callers can check post-construction?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode311
content/common/gpu/media/vaapi_h264_decoder.h:311: void
UnbindFromTexture();
inline into dtor

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode315
content/common/gpu/media/vaapi_h264_decoder.h:315: bool Sync();
not defined?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode317
content/common/gpu/media/vaapi_h264_decoder.h:317: // Initialize/destroy
static state of this class (X Display and current
Why static? Instead leave ownership w/ decoder object and pass refs in
ctor. Then decoder is in charge of init/destroy.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode325
content/common/gpu/media/vaapi_h264_decoder.h:325: static
base::LazyInstance<GLXFBConfig> fb_config_;
I think LazyInstance can be dropped.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode338
content/common/gpu/media/vaapi_h264_decoder.h:338: typedef std::map<int,
DecodeSurface*> POCToDecodeSurfaces;
why not a linked_ptr?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode361
content/common/gpu/media/vaapi_h264_decoder.h:361: // Assing a surface
to PicOrderCount, removing it from the available surfaces
s/Assing/Assign/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode363
content/common/gpu/media/vaapi_h264_decoder.h:363: // to decode too.
s/too/to/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc#newcode128
content/common/gpu/media/vaapi_video_decode_accelerator.cc:128: void
VaapiVideoDecodeAccelerator::RequestPictureBuffers(int num_pics,
Why not pass the VDA::Client to the decoder, so it can call
RequestPictureBuffer, PictureReady, etc directly?
(I suspect having these trampolines here is only adding code &
indirection, unnecessarily)

Once you do that I think there's some cleanup that can be done (i.e.
decoder's pic_width/height() are no longer necessary, as is
GetRequiredNumOfPics()).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc#newcode442
content/common/gpu/media/vaapi_video_decode_accelerator.cc:442:
base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,
Wait, wat??
decoder tells VAVDA a pic is ready but then VAVDA has to ask decoder to
sync the texture? Isn't that crazy? Why wouldn't decoder sync the
texture before telling VAVDA it's ready?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.gypi#newcode386
content/content_common.gypi:386: 'common/gpu/media/h264_dpb.cc',
l.386-389 are not cros/x64-specific, are they?
(move to the unconditional sources list above, even though they won't be
used by non-vaapi-containing builds?)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi#newcode493
content/content_tests.gypi:493:
'common/gpu/media/video_decode_accelerator_unittest.cc',
Were you able to make this test work w/ VAVDA?
(IWBN if this CL included a change to this stanza to make that work)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi#newcode498
content/content_tests.gypi:498: ['chromeos == 1', {
Not part of this CL

https://chromiumcodereview.appspot.com/9814001/diff/1/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/public/common/content_switches.cc#newcode342
content/public/common/content_switches.cc:342: const char
kEnableVaapiDecodeAcceleration[] = "enable-vaapi-decode-accel";
Can VAAPI be used for anything other than video decode acceleration?
I think a better flag name would be --enable-vaapi / kEnableVaapi

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2158
content/renderer/render_view_impl.cc:2158: // implementations are added,
relax the #if above.
s/above/below/

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2160
content/renderer/render_view_impl.cc:2160: && (defined(ARCH_CPU_ARMEL)
|| defined(ARCH_CPU_X86_FAMILY))
&& belongs on previous line

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2161
content/renderer/render_view_impl.cc:2161: // Currently only cros has
any HW video decode support in
delete?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2164
content/renderer/render_view_impl.cc:2164:
WebGraphicsContext3DCommandBufferImpl* context3d =
Why bother going all the way to the GPU process to find out VAAPI is
disabled? I'd check for the enable flag here, instead.
(that would also let you not propagate the enable flag to the GPU
process, reverting gpu_process_host.cc)

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium
File third_party/libva/README.chromium (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium#newcode1
third_party/libva/README.chromium:1: Name: libva
Is there some reason to copy libva into third_party instead of pulling
it in via DEPS?

Hint: I think the answer is "no", unless you have local modifications
that must be applied to the third-party code that can't be upstreamed.
If that's the case then those modifications deserve description here.
I hope you can avoid this with a suitable addition to DEPS (and a
corresponding crbug to get a git mirror set up for UsingNewGit to work)

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium#newcode4
third_party/libva/README.chromium:4: Security Critical: yes
Do you know what this line does? (I don't)

https://chromiumcodereview.appspot.com/9814001/diff/1/ui/gfx/gl/gl_context_egl.h
File ui/gfx/gl/gl_context_egl.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/ui/gfx/gl/gl_context_egl.h#newcode36
ui/gfx/gl/gl_context_egl.h:36: virtual EGLDisplay GetDisplay() OVERRIDE;
I think the ui/ part of this CL should be pulled out and be its own CL
(since it's so independent).

However, it's not clear to me you need these changes, anyway.
In both GLX/EGL the relevant display is a static property, which you can
get with
base::MessagePumpForUI::GetDefaultXDisplay();
and
eglGetDisplay(EGL_DEFAULT_DISPLAY);
resp.

(also, AFAICT, you only actually use the GLX version, so the EGL edit is
completely unneeded?)

https://chromiumcodereview.appspot.com/9814001/

pos...@chromium.org

unread,

Mar 21, 2012, 2:40:16 PM3/21/12

to fisc...@chromium.org, chromium...@chromium.org, dhollow...@chromium.org, j...@chromium.org, apat...@chromium.org, joi+watc...@chromium.org, feature-me...@chromium.org, dari...@chromium.org

On 2012/03/21 13:16:23, Ami Fischman wrote:
> I ran out of code-reviewing steam at vaapi_h264_decoder.cc:1739 (and
> before
> looking through VAVDA.{h,cc}).
> I'll hold off looking at the rest until after you address previous
> comments.
> Please make an effort to bring the whole CL into style-guide compliance,
> as it
> makes it much harder than it needs to be to review this code as-is.

Wow, you really scared me with this comment at first. I tend to think of
myself
as a very meticulous person ;)

I'm sorry that parser got included here, my misunderstanding of how rietveld
worked (different from gerrit). I committed it to the original parser review
first, but this got squashed back...
I think many of your style comments are due to me trying to keep faithful to
spec naming, as I mentioned in the inline replies. We have to make a
decision
about this please.

> Also, please make an effort to reduce duplication of text where possible
> (I
made
> a couple of comments to this effect elsewhere, but wanted to call this out
> specifically as something to think about at the whole-CL level.

Sorry, I don't recall any comments about duplication after reading the
review.
Could you point out an example please if you remember one?

> Finally, please make a pass in rietveld to ensure code you meant to drop
> is
gone
> (commented-out lines), and TODOs are either in proper format
> ("TODO(posciak):
> what needs to be done and why") or are gone (i.e. no "TODO posciak think
> about
> this") :)

There were perhaps some 5-10 commented-out lines I wanted to keep as future
reference. I will make sure to document properly.

https://chromiumcodereview.appspot.com/9814001/

pos...@chromium.org

unread,

Mar 21, 2012, 2:40:35 PM3/21/12

to fisc...@chromium.org, chromium...@chromium.org, dhollow...@chromium.org, j...@chromium.org, apat...@chromium.org, joi+watc...@chromium.org, feature-me...@chromium.org, dari...@chromium.org

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc
File content/common/gpu/media/h264_dpb.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc#newcode132

content/common/gpu/media/h264_dpb.cc:132: return
*(std::min_element(short_refs.begin(), short_refs.end(),

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Ow, that hurted my brain!

Blame STL :) Do you see a way to make it simpler? Or do you want to
return an iterator?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode28
content/common/gpu/media/h264_dpb.h:28: int TopFieldOrderCnt;

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Naming in this file is inconsistent. Please style-guide it up.

This is intentional and deliberate. I wanted to keep the naming
consistent with the spec, so people familiar with it could follow the
code easier. There is a good number of variables that are this way in
the spec, for example there are FrameNum and frame_num, which are
different variables. I decided this violation of Chromium style would be
beneficial for people familiar with the spec, or anyone who would run a
search in the spec for each variable have a match with the definition of
the variable.
If you feel this is not enough of a justification, I'll have to come up
with a different naming convention.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode56
content/common/gpu/media/h264_dpb.h:56: // Utility function objects

On 2012/03/21 13:16:24, Ami Fischman wrote:
> These make me sad. Are they really all needed?
> Can they be defined near their only use if they're only used in one
(.cc) file?

Some are used in two different .cc files. I had them in .cc originally,
but then I separated DPB and Decoder classes into two files, so I had to
decide whether to keep the shared ones here and rest in .cc, or be more
consistent and not scatter things around and keep all of them here. If
you feel the former is better, I'll move it back.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode118
content/common/gpu/media/h264_dpb.h:118: class H264DPB {

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Sad that the spec defines DPB as a buffer of decoded pictures, but
other VDA
> terminology uses PictureBuffer for a single image :(

Again, a lot of this is dictated by the idea to keep things consistent
with the spec so people can look things up in there easily.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode133
content/common/gpu/media/h264_dpb.h:133: void StorePic(H264Picture*
pic);

On 2012/03/21 13:16:24, Ami Fischman wrote:
> If you passed by scoped_ptr you wouldn't have to document ownership
transfer.

Maybe I'm too hanged on the Google C++ style recommendation to use plain
pointer containers when possible and just have a strong specification of
ownership.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.h
File content/common/gpu/media/h264_parser.h (right):

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Not part of this CL?

Yeah, looks like I was misunderstanding how rietveld and git cl upload
worked. I've been told it would only pick up the top commit as it
associates branch with review #.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc
File content/common/gpu/media/vaapi_h264_decoder.cc (right):

On 2012/03/21 13:16:24, Ami Fischman wrote:
> this name is confusing
> (why, yes, I'm sure we either had success or had an error ;))

Well, maybe I shouldn't have abbreviated it that much. I means
"VA_SUCCESS_OR_REPORT_AN_ERROR".

On 2012/03/21 13:16:24, Ami Fischman wrote:
> this is a lie during the call from the ctor

But ctor changes it back... I was thinking of separating Reset() and
construction completely, but didn't want to duplicate code. Maybe I
should have anyway

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode268
content/common/gpu/media/vaapi_h264_decoder.cc:268: switch (state_) {

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Reset() just set state_; why is this switch necessary?

In case we call it without Reset()?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode473
content/common/gpu/media/vaapi_h264_decoder.cc:473: XSync(x_display_,
False);

On 2012/03/21 13:16:24, Ami Fischman wrote:
> I don't think it's ok to have XSync's without documented reason (I
expect them
> not to be needed).

Yes, this will not be here in the final version, just didn't have time
to properly investigate this.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode508
content/common/gpu/media/vaapi_h264_decoder.cc:508: DCHECK_EQ(state_,
kDecoding);

On 2012/03/21 13:16:24, Ami Fischman wrote:
> how can you be sure? What if an error occurred but was only delivered
after the
> AssignPictureBuffer task was queued?

There can be no threading here yet.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode549
content/common/gpu/media/vaapi_h264_decoder.cc:549: pic_height_,
VA_RT_FORMAT_YUV420,

On 2012/03/21 13:16:24, Ami Fischman wrote:
> indentation here and elsewhere is off. please make a pass.

Chromium coding style says "do whatever seems most readable", so this
was my, perhaps poor, idea of best readability ;)

On 2012/03/21 13:16:24, Ami Fischman wrote:
> what's the reverse order about?

vaapi documentation is nonexistent, literally (well, apart from a few
sparse comments in the header, which are not enough at all. I could only
experiment and try to follow what other implementations were doing when
in doubt.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> not worried about overwriting a previous value?
> (why use operator[] instead of .insert() + DCHECK ret val?

Not that much, because I checked for ->available == false. But DCHECKS
are always good.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode667
content/common/gpu/media/vaapi_h264_decoder.cc:667: va_pic->picture_id =
0xffffffff;

On 2012/03/21 13:16:24, Ami Fischman wrote:
> kuint32max?
> (but why set it to this?)

Again, not documented, but empirical from what other implementations do.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> I'm weeping over here over the amount of text involved in this. No
better way?
> For example, is it a bad idea to have the parser emit to the
VA-specific
> structures to avoid all this manual copying?
> (and as a nice side-effect, would allow you to drop the lengthy
structs from the
> parser's definition).

I've been weeping too :( Unfortunately, some variables are named
differently from spec and are scattered across bit fields, which is not
per spec either. The whole structure does not really map exactly to spec
structures either. There is no pic param buffer in spec, neither there
is a slice param or iq matrix, and they use data from different spec
structures and also some calculated by decoder ones, parser has no way
to know. Otherwise there would be no need for decoder class at all.
I also wanted to keep the same names as in spec in the parser for
obvious reasons.
I didn't want to make the parser vaapi-specific either, only this class.
I could have some macros generating this thing though. Would that be
preferable?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode783
content/common/gpu/media/vaapi_h264_decoder.cc:783: for (i = 0; i < 2;
++i)

On 2012/03/21 13:16:24, Ami Fischman wrote:
> I'm surprised by the '2' here and at l.793, expecting to see 6's.

Yes, I was surprised with this asymmetry as well on first reading of the
spec, this is not an error though.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> What's your plan here?

Small optimization that may save us a little bit, so low priority.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> If 32 is a real limit, why not just maintain pending_va_bufs_ &
> pending_slice_bufs_ as VABufferID[kMaxVABuffers] along with
> pending_{va,slice}_count_, and avoid this business with having to copy
them over
> here?

I had it that way originally actually. The problem is, I don't know the
limit. But I probably over-engineered this. Still, this is just a
one-int copy, not copying any struct or class, so I thought it'd be
negligible as opposed to checking bounds every time I added to the
structure, removed from it and risking forgetting somewhere in more
complicated cases...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1017
content/common/gpu/media/vaapi_h264_decoder.cc:1017: frame_decoded_ =
true;

On 2012/03/21 13:16:24, Ami Fischman wrote:
> this var name and the comment preceding it are at odds.
> At this point in time has the frame been decoded or not?

From our standpoint yes, because we committed all the data and only the
HW has it. We can discard it ourselves. We can also report that its
ready and can be requested straight from the hardware, even if the
request were to block. I'm not syncing with HW here yet for obvious
speed reasons.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1037
content/common/gpu/media/vaapi_h264_decoder.cc:1037: // Not correct if
not a field.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> not sure what this means.

The code here takes advantage of knowledge that it doesn't have to take
into account interlaced cases. But I left comments about this here and
in other places to indicate some some code was making that assumption in
case we wanted to add support for interlaced.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> This is a COMPILE_ASSERT that can go somewhere (not in the decode
code-path)?

But only in debug build, but you are right.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> since only case 0 is handled it would be much clearer (and allow less
> indentation) if you checked for the != 0 case and errored-out early.

Probably... I would like to add this, but later on, so I didn't want to
fold it back... I'll probably just add it...

On 2012/03/21 13:16:24, Ami Fischman wrote:
> unclear

This is one of the markers saying I'm cutting corners assuming
non-interlaced. But you are right.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> This (and below) is an awful lot of std::sort()'ing.
> Is it really necessary?

Sorts are done on different comparators/different members of
H264Picture. Since the number of elements is <=16 (<=32), I thought it'd
be negligible.
We could replace the vector with a multi-indexed set class or 3-4
ordered set structures holding pointers to the same pictures but keeping
them in different orders. Worth the complexity for 16-32 elements?

On 2012/03/21 13:16:24, Ami Fischman wrote:
> indent is wrong here & elsewhere
> operators belong on preceding line, always.

Yeah, I really can't make peace with this I guess :)

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Can you try to extract common functionality into helper functions to
avoid
> duplicate structures?

Well, again, this is intentional. This is exactly as spec'ed. Exact code
from spec, to ensure it's correct and easily verifiable. When deciding
whether to optimize, I always put exactness with the spec before
optimized and modified code if the gains looked small or were only to
improve readability. For a person looking at spec, they could just
compare this verbatim (hence my links to relevant spec chapters too).

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Replace this comment with a DCHECK at the top of the function.

I didn't want to pollute this class with threading stuff, that's the
biggest reason for separating this class from VAVDA. I DCHECK in the
caller, in VAVDA, before calling this function instead. I'd prefer
keeping it this way, it's already pretty complicated here.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> What is this doing?
> The comment reads to me like "copy from the VA surface to the pixmap"
but I
> thought you said this is zero-copy. What's up?

This is driver/platform-dependent and implemented by it.
The real 100% no-copy-at-all-evar can only be guaranteed if we are not
passing stuff back to GL (not even mentioning SW compositor), but
displaying it directly from here with vaapi calls, not GL.

That said though, since GL and VAAPI is the same Intel driver and same
device (non-discrete GPU), it would be a horrible omission if they did a
copy here.

Moreover, in SW decoding case it's not really a zero-copy, since when we
write to the texture buffer bound to a pixmap with a software decoder,
the data has to be uploaded to the GPU.

In this case here in VAAPI, it is different from SW, here when we
combine VAAPI with GL, if this was to be a copy, it'd be a copy inside
the GPU, from GPU to GPU, and not from CPU to GPU memory. Both VASurface
and the texture are already in GPU memory and not touchable from
software. So this should always be better than current zero-copy, unless
we start touching with SW compositor or anything CPU mapped.

Of course the above is still platform/driver dependent, whether they
have it optimized correctly (yet) and a result of logical reasoning, and
not of a deep dive into the driver and hardware.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Why do it this way?

Because I want to postpone Sync until the last possible moment to
minimize potential blocking on it. And because it has to be done on the
GLX thread, not the decoder thread.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1464
content/common/gpu/media/vaapi_h264_decoder.cc:1464: DCHECK(msg_loop_);

On 2012/03/21 13:16:24, Ami Fischman wrote:
> I thought you were going to PostTask to this msg loop, not DCHECK it
here.

I'm overgenerous with DCHECKs sometimes, but I have a feeling we think
of them in a different way. For me they are noops whenever performance
is cared about at all, and that I shouldn't care about performance when
they are present. Am I missing something?

On 2012/03/21 13:16:24, Ami Fischman wrote:
> comment is conditional but next two lines aren't.

Yeah, it refers to the ifs below as well.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h
File content/common/gpu/media/vaapi_h264_decoder.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode12
content/common/gpu/media/vaapi_h264_decoder.h:12: #include
"h264_parser.h"

On 2012/03/21 13:16:24, Ami Fischman wrote:
> these includes are unstylish (belong in order below, with
> content/common/gpu/media/ prefix)

Oh, I thought those fell into tightly related modules category, which
should be on top. Ok, will fix.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode21
content/common/gpu/media/vaapi_h264_decoder.h:21: #include
"base/memory/linked_ptr.h"

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Any reason you didn't use linked_ptr for PicsVector?

Yes, this:
http://www.corp.google.com/eng/doc/devguide/cpp/primer.html#google_pointers
(paragraph a bit further down about linked_ptr). "linked_ptr: avoid in
new code" and it also mentions plain pointer containers are fine and
I've seen plenty of those in Chrome code, so I thought they were a valid
alternative.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> is this really a concern of this class?

Well, binding is related to VASurfaces. I can have a subclass, but it'd
still kind of require association of pixmaps with VASurfaces, so I
thought separating this wasn't worth adding an additional map of
VASurface->"pixmap related things". We already have PoC->dec_surface and
output_buffer_id->dec_surface.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Is there some reason not to refer to this as the ChildThread
(chromium-wide
> convention & class)?

Not sure if we are on the same page. GLX thread is the parent thread.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> I think this is a problematic contract. Is there some reason not to
annotate
> every public function with "Call on ChildThread." vs. "Call on decode
thread."?

No and I'm annotating everything anyway. I'll remove this.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Is msg_loop the decoder thread or the child thread?
> If the latter, then why not use MessageLoop::current() in the impl of
Initialize
> (given you require Initialize must be called on the child thread)?

Decoder thread is the child thread. The main thread is the GLX thread.
Initialize is to be called on the main (i.e. GLX thread).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode85
content/common/gpu/media/vaapi_h264_decoder.h:85: // TODO reorganize
private/public

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Yes, please. (it's hard to review this class without being sure what
its API
> is)

Haha, can't believe I left this here ;)

On 2012/03/21 13:16:24, Ami Fischman wrote:
> See comment elsewhere, but hopefully this can be inlined into
OutputPic() or
> whatever ends up calling the picture-ready callback.

I'll respond in the other place.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode103
content/common/gpu/media/vaapi_h264_decoder.h:103: bool Flush();

On 2012/03/21 13:16:24, Ami Fischman wrote:
> what's the ret value?

Could've errored out at OutPic. I'll document.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> What state needs to be kept across Reset()? I am surprised that any
state is
> useful at that point.

Quite a bit, actually. SPSes and PPSes have to be kept and related
variables as well. It's possible for the entire stream to have a single
PPS and SPS, so if we didn't keep them, we wouldn't be able to resume
from anywhere at all (or would have to re-read the stream from the very
beginning).
This is actually the main reason why reset and resume from random place
is quite complicated.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Why is input_id a param to this & DecodeOneFrame instead of being a
param to
> SetStream?

To support having more than one frame in one stream chunk (as we
discussed we didn't want to make an assumption we wouldn't).

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Isn't this an excessive amount of memory to be holding on to?

I need to be able to keep at least H264DPB::kDPBMaxSize of frames for
reference and delayed output, in other words that many pictures kept in
decoder at once and not given back to the client. This is per spec.
+6 was empirical, based on how much the client held on to before it
actually started giving back (although I might have gotten away with
+5).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode221
content/common/gpu/media/vaapi_h264_decoder.h:221:
scoped_ptr<H264Picture> curr_pic_;

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Why not a simple member?

You mean a pointer?

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Does it not make sense to have a
> H264Picture last_ref_picture_;
> ?

I was deliberating it for quite some time, but I felt that associated
management pain wasn't worth it (not mentioning wasted memory). The last
ref picture doesn't have to still be present in DPB or anywhere, so it's
not just a question of pointing to something that is in DPB.
I guess a refcounted ptr would solve this though..., but would still
keep memory and maybe complicate management of associated picture buffer
(texture), although I might have to revisit that last claim...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode284
content/common/gpu/media/vaapi_h264_decoder.h:284: struct DecodeSurface
{

On 2012/03/21 13:16:24, Ami Fischman wrote:
> There is private state and methods in this, so it's a class, not a
struct (and
> fields need to be renamed & set w/ setters and read with getters).
But I think
> most of the fields are read-only and could be set in the ctor.

Yeah... I really hoped to avoid class and getter/setter bloat. I guess
no running away from it :(

On 2012/03/21 13:16:24, Ami Fischman wrote:
> inline into ctor and replace bool ret value with setting available to
false,
> which callers can check post-construction?

The class is constructed before we can bind, that's why this is separate
and not called from the constructor. VASurfaces are allocated in one go
in one driver call and a context is associated with all of them. If I
were to construct here, I'd have to allocate VASurfaces one by one and
then gather them back and associate context with it, which if it'd fail
would be harder to report too. And I would have to trigger this somehow
after all BindToTexture() calls are done. So I felt this way would be
much more clear.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Why static? Instead leave ownership w/ decoder object and pass refs
in ctor.
> Then decoder is in charge of init/destroy.

Only because it isn't related to decoder, just to decode surfaces. I can
put it back.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> why not a linked_ptr?

See my previous comment, but I guess you really prefer it... :)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Why not pass the VDA::Client to the decoder, so it can call
> RequestPictureBuffer, PictureReady, etc directly?
> (I suspect having these trampolines here is only adding code &
indirection,
> unnecessarily)

> Once you do that I think there's some cleanup that can be done (i.e.
decoder's
> pic_width/height() are no longer necessary, as is
GetRequiredNumOfPics()).

I was really trying not to add additional complexity to the decoder
class. It's complicated already I feel. I don't want people reviewing
and verifying the decoder to bother with VDA and threading details to
confuse them even more. This is the main reason I abstracted what I
could to VAVDA class, this includes threading, talking to the Client,
etc.

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Wait, wat??
> decoder tells VAVDA a pic is ready but then VAVDA has to ask decoder
to sync the
> texture? Isn't that crazy? Why wouldn't decoder sync the texture
before
> telling VAVDA it's ready?

Decoder can't sync, because it has to be done on GLX thread. I also want
to postpone sync as much as possible and not stall the decoder thread
for it, it has enough to do anyway.
VAVDA gets this callback on decoder thread which goes back happily on
its way. VAVDA has to switch go GLX thread, which could be doing other
things. Don't need to stall the decoder. And finally, the HW has more
time to decode as well in the meantime (which is kind of a third thread
in this conceptual model).

I also don't want the decoder to bother with threading as I mentioned
before.

So in other words, this lets both the decoder (software parsing, etc)
and HW (hardware decode) not have to wait for each other and for the VDA
client. Otherwise the three of them, HW, decoder thread and GLX thread
would have to converge at one point. Yuck :)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.gypi#newcode386
content/content_common.gypi:386: 'common/gpu/media/h264_dpb.cc',

On 2012/03/21 13:16:24, Ami Fischman wrote:
> l.386-389 are not cros/x64-specific, are they?
> (move to the unconditional sources list above, even though they won't
be used by
> non-vaapi-containing builds?)

Do we want to compile them in if they are not used?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi#newcode493
content/content_tests.gypi:493:
'common/gpu/media/video_decode_accelerator_unittest.cc',

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Were you able to make this test work w/ VAVDA?
> (IWBN if this CL included a change to this stanza to make that work)

It requires adding GLX support and is not trivial. But I will definitely
do it.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Can VAAPI be used for anything other than video decode acceleration?

> I think a better flag name would be --enable-vaapi / kEnableVaapi

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2160

content/renderer/render_view_impl.cc:2160: && (defined(ARCH_CPU_ARMEL)
|| defined(ARCH_CPU_X86_FAMILY))

On 2012/03/21 13:16:24, Ami Fischman wrote:
> are there any cros platforms *other* than x86 & arm?

Not that I am aware of. Wanted to be extra safe. Drop it?

On 2012/03/21 13:16:24, Ami Fischman wrote:
> delete?

Your call, was here before :)

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium
File third_party/libva/README.chromium (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium#newcode1
third_party/libva/README.chromium:1: Name: libva

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Is there some reason to copy libva into third_party instead of pulling
it in via
> DEPS?

> Hint: I think the answer is "no", unless you have local modifications
that must
> be applied to the third-party code that can't be upstreamed.

We are actually thinking with Stephane about this to optimize the driver
ourselves, but not for now at least.

> If that's the case
> then those modifications deserve description here.
> I hope you can avoid this with a suitable addition to DEPS (and a
corresponding
> crbug to get a git mirror set up for UsingNewGit to work)

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium#newcode4
third_party/libva/README.chromium:4: Security Critical: yes

On 2012/03/21 13:16:24, Ami Fischman wrote:
> Do you know what this line does? (I don't)

No, so erring on the side of caution. :) But since it's executed on the
GPU thread, I thought this would be wise...

https://chromiumcodereview.appspot.com/9814001/diff/1/ui/gfx/gl/gl_context_egl.h
File ui/gfx/gl/gl_context_egl.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/ui/gfx/gl/gl_context_egl.h#newcode36
ui/gfx/gl/gl_context_egl.h:36: virtual EGLDisplay GetDisplay() OVERRIDE;

On 2012/03/21 13:16:24, Ami Fischman wrote:
> I think the ui/ part of this CL should be pulled out and be its own CL
(since
> it's so independent).

> However, it's not clear to me you need these changes, anyway.
> In both GLX/EGL the relevant display is a static property, which you
can get
> with
> base::MessagePumpForUI::GetDefaultXDisplay();
> and
> eglGetDisplay(EGL_DEFAULT_DISPLAY);
> resp.

> (also, AFAICT, you only actually use the GLX version, so the EGL edit
is
> completely unneeded?)

Yes, it will be the case in the final CL, just didn't want to give you
something that wouldn't compile and my understanding was that I can't
have dependencies between CLs here, can I?

https://chromiumcodereview.appspot.com/9814001/

Ami Fischman

unread,

Mar 22, 2012, 10:53:06 AM3/22/12

to pos...@chromium.org, fisc...@chromium.org, chromium...@chromium.org, dhollow...@chromium.org, j...@chromium.org, apat...@chromium.org, joi+watc...@chromium.org, feature-me...@chromium.org, dari...@chromium.org

On Wed, Mar 21, 2012 at 7:40 PM, <pos...@chromium.org> wrote:

I'm sorry that parser got included here, my misunderstanding of how rietveld
worked (different from gerrit). I committed it to the original parser review
first, but this got squashed back...

Trailing args to [git cl upload] are args to git diff, so for example if you have branch1 & branch2, then you can upload just the 1->2 diff by saying

git cl upload branch1..branch2

Note that git cl ought to do the right thing if branch2 tracks branch1 as its upstream (plus then it's easier to rebase in changes to branch1 that develop during CR).

I think many of your style comments are due to me trying to keep faithful to
spec naming, as I mentioned in the inline replies. We have to make a decision
about this please.

During the parser CL I was implicitly assuming that the spec naming business would be confined to the parser and that the decoder and VDA would be able to avoid knowing about that naming scheme.

But now I see that was a bad assumption (esp. since the parser structs are all structs, so their consumers _must_ know about their naming conventions).

IMO we should drop the spec naming conventions and use chromium conventions everywhere; this code will be read much more by people hacking on chromium than people hacking on the spec.

So e.g. BottomFieldOrderCnt becomes bottom_field_order_count_

Also, please make an effort to reduce duplication of text where possible (I made

a couple of comments to this effect elsewhere, but wanted to call this out
specifically as something to think about at the whole-CL level.
Sorry, I don't recall any comments about duplication after reading the review.
Could you point out an example please if you remember one?

Scanning my comments this is the only one I found:

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1383

When I wrote the CL-level comment I had the feeling that I had been seeing more extractable/de-dup'able code.

Finally, please make a pass in rietveld to ensure code you meant to drop is gone

(commented-out lines), and TODOs are either in proper format ("TODO(posciak):
what needs to be done and why") or are gone (i.e. no "TODO posciak think about
this") :)
There were perhaps some 5-10 commented-out lines I wanted to keep as future
reference. I will make sure to document properly.

FWIW, generally chromium code strongly discourages commented-out code as documentation. It's almost always better to involve that code in a unit test as an instructive example (that won't go stale/misleading at the first change of the code post-landing).

fisc...@chromium.org

unread,

Mar 22, 2012, 1:01:36 PM3/22/12

to pos...@chromium.org, chromium...@chromium.org, dhollow...@chromium.org, j...@chromium.org, apat...@chromium.org, joi+watc...@chromium.org, feature-me...@chromium.org, dari...@chromium.org

Please let me know when you have uploaded a new version of the patch.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc
File content/common/gpu/media/h264_dpb.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.cc#newcode132

content/common/gpu/media/h264_dpb.cc:132: return
*(std::min_element(short_refs.begin(), short_refs.end(),

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Ow, that hurted my brain!

> Blame STL :) Do you see a way to make it simpler? Or do you want to
return an
> iterator?

why not the much simpler

H264Picture* ret = NULL;

for (int i = 0; i < pics_.size(); ++i) {

if (!ret || pics_[i]->FrameNum < ret->FrameNum)
ret = pics_[i];
}
return ret;
?
it's 1 line shorter, avoids allocating & copies populating short_refs,
and avoids the extra iteration over short_refs.

(this is a prime example of what I meant by replace the use of
algorithms with the loop-based alternative).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h
File content/common/gpu/media/h264_dpb.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode28
content/common/gpu/media/h264_dpb.h:28: int TopFieldOrderCnt;

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Naming in this file is inconsistent. Please style-guide it up.

> This is intentional and deliberate. I wanted to keep the naming
consistent with
> the spec, so people familiar with it could follow the code easier.
There is a
> good number of variables that are this way in the spec, for example
there are
> FrameNum and frame_num, which are different variables. I decided this
violation
> of Chromium style would be beneficial for people familiar with the
spec, or
> anyone who would run a search in the spec for each variable have a
match with
> the definition of the variable.
> If you feel this is not enough of a justification, I'll have to come
up with a
> different naming convention.

Yeah, I think making this code friendly to spec-experts is the wrong way
to go (though I understand your motivation).
In reality we're going to have far far more people looking at this code
who expect/understand chromium naming than h264 naming (unfortunately).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode56

content/common/gpu/media/h264_dpb.h:56: // Utility function objects

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > These make me sad. Are they really all needed?
> > Can they be defined near their only use if they're only used in one
(.cc)
> file?

> Some are used in two different .cc files. I had them in .cc
originally, but then
> I separated DPB and Decoder classes into two files, so I had to decide
whether
> to keep the shared ones here and rest in .cc, or be more consistent
and not
> scatter things around and keep all of them here. If you feel the
former is
> better, I'll move it back.

Yes, I think it's better to define them near their uses.
(although, see my other comments about algorithms; I feel at least most
of the code using these is less readable (and not much shorter,
sometimes longer) than the more straightforward loop-based alternative)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_dpb.h#newcode133

content/common/gpu/media/h264_dpb.h:133: void StorePic(H264Picture*
pic);

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > If you passed by scoped_ptr you wouldn't have to document ownership
transfer.

> Maybe I'm too hanged on the Google C++ style recommendation to use
plain pointer
> containers when possible and just have a strong specification of
ownership.

scoped_ptr<>'s .Pass() is the strongest specification of ownership we
have (certainly much stronger than comments, which are not checked by a
compiler).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.h
File content/common/gpu/media/h264_parser.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/h264_parser.h#newcode2

content/common/gpu/media/h264_parser.h:2: // Use of this source code is
governed by a BSD-style license that can be

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Not part of this CL?

> Yeah, looks like I was misunderstanding how rietveld and git cl upload
worked.
> I've been told it would only pick up the top commit as it associates
branch with
> review #.

Addressed this elsewhere, but FWIW it's always a good idea to look at
rietveld to see what was actually uploaded (before sending the review
request) :)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc
File content/common/gpu/media/vaapi_h264_decoder.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode16

content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_SUCCESS_OR_ERROR(vares, err_msg) \

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > this name is confusing
> > (why, yes, I'm sure we either had success or had an error ;))

> Well, maybe I shouldn't have abbreviated it that much. I means
> "VA_SUCCESS_OR_REPORT_AN_ERROR".

I know; my problem with the name is that you're not actually reporting
an error (state_=kError); you're only DLOG'ing it...
VA_LOG_ON_ERROR would work for me.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode259

content/common/gpu/media/vaapi_h264_decoder.cc:259: // Still initialized
and ready to decode

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > this is a lie during the call from the ctor

> But ctor changes it back... I was thinking of separating Reset() and
> construction completely, but didn't want to duplicate code. Maybe I
should have
> anyway

I think your instinct to avoid duplication was correct, and it's nice to
hvae ctor's call Reset.
I'm just saying the comment is misleading; fix that by appending ",
unless called from the ctor, which will reset this" to the comment.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode268
content/common/gpu/media/vaapi_h264_decoder.cc:268: switch (state_) {

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Reset() just set state_; why is this switch necessary?

> In case we call it without Reset()?

You unconditionally call Reset() two lines up, though.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode508
content/common/gpu/media/vaapi_h264_decoder.cc:508: DCHECK_EQ(state_,
kDecoding);

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > how can you be sure? What if an error occurred but was only
delivered after
> the
> > AssignPictureBuffer task was queued?

> There can be no threading here yet.

But a previously-failed APB() could have set error.
(well, it would, if VAVDA::APB checked the return code of this function,
which it appears not to)

In the future APB will be callable mid-stream to support frame-size
changes.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode549
content/common/gpu/media/vaapi_h264_decoder.cc:549: pic_height_,
VA_RT_FORMAT_YUV420,

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > indentation here and elsewhere is off. please make a pass.

> Chromium coding style says "do whatever seems most readable", so this
was my,
> perhaps poor, idea of best readability ;)

Umm, yeah, no. ;)
Your choices are
blah(foo1,
foo2);
or
blah(
foo1,
foo2);

(I.e. ascii-art indent or 4-space indent; can't mix-and-match)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode613

content/common/gpu/media/vaapi_h264_decoder.cc:613: // Return reference
frames in reverse order of insertion

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > what's the reverse order about?

> vaapi documentation is nonexistent, literally (well, apart from a few
sparse
> comments in the header, which are not enough at all. I could only
experiment and
> try to follow what other implementations were doing when in doubt.

When you have code that even you don't understand you should assume
every other reader of the code will be even more perplexed than you, and
add descriptive ("why") comments.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode686

content/common/gpu/media/vaapi_h264_decoder.cc:686:
pic_param.picture_width_in_mbs_minus1 = sps->pic_width_in_mbs_minus1;

Can the parser not populate as much as it can and then have this class
populate the decoder-discovered bits?

> I also wanted to keep the same names as in spec in the parser for
obvious
> reasons.

Understood, but see elsewhere.

> I didn't want to make the parser vaapi-specific either, only this
class.

This reason is the least-compelling to me.

> I could have some macros generating this thing though. Would that be
preferable?

If they result in significantly fewer lines and more
demonstrably-correct code (by avoiding the need to repeat field names
that are identical) then hell yes.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode783

content/common/gpu/media/vaapi_h264_decoder.cc:783: for (i = 0; i < 2;
++i)

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > I'm surprised by the '2' here and at l.793, expecting to see 6's.

> Yes, I was surprised with this asymmetry as well on first reading of
the spec,
> this is not an error though.

Are the other 4 rows left untouched??

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode985

content/common/gpu/media/vaapi_h264_decoder.cc:985: buffers[i] =
pending_va_bufs_.front();

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > If 32 is a real limit, why not just maintain pending_va_bufs_ &
> > pending_slice_bufs_ as VABufferID[kMaxVABuffers] along with
> > pending_{va,slice}_count_, and avoid this business with having to
copy them
> over
> > here?

> I had it that way originally actually. The problem is, I don't know
the limit.

If that's the problem, I'm not seeing a solution :)

> But I probably over-engineered this. Still, this is just a one-int
copy, not
> copying any struct or class, so I thought it'd be negligible

It is indeed negligble.

> as opposed to
> checking bounds every time I added to the structure, removed from it
and risking
> forgetting somewhere in more complicated cases...

But you never do anything to the vectors other than push to their back
and later clear.
ISTM like a safe change that will uniformly increase readability,
simplicity, and robustness.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1017

content/common/gpu/media/vaapi_h264_decoder.cc:1017: frame_decoded_ =
true;

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > this var name and the comment preceding it are at odds.
> > At this point in time has the frame been decoded or not?

> From our standpoint yes, because we committed all the data and only
the HW has
> it. We can discard it ourselves. We can also report that its ready and
can be
> requested straight from the hardware, even if the request were to
block. I'm not
> syncing with HW here yet for obvious speed reasons.

Then s/frame_decoded_/frame_ready_at_hw_/

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1037

content/common/gpu/media/vaapi_h264_decoder.cc:1037: // Not correct if
not a field.

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > not sure what this means.

> The code here takes advantage of knowledge that it doesn't have to
take into
> account interlaced cases. But I left comments about this here and in
other
> places to indicate some some code was making that assumption in case
we wanted
> to add support for interlaced.

I think these comments (about "field"s) could be clarified to be more
informative to the less spec-aware.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1057
content/common/gpu/media/vaapi_h264_decoder.cc:1057:
DCHECK_EQ(sizeof(curr_pic_->ref_pic_marking),

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > This is a COMPILE_ASSERT that can go somewhere (not in the decode
code-path)?

> But only in debug build, but you are right.

COMPILE_ASSERT should not be in only debug build. It's a build-time
assertion (so there is no run-time cost) and should be applied to all
build configurations (since debug & release can differ in subtle ways,
and you want to guarantee the invariants you rely on in all
configurations).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1188

content/common/gpu/media/vaapi_h264_decoder.cc:1188:
std::sort(RefPicList0_.begin(), RefPicList0_.end(),

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > This (and below) is an awful lot of std::sort()'ing.
> > Is it really necessary?

> Sorts are done on different comparators/different members of
H264Picture. Since
> the number of elements is <=16 (<=32), I thought it'd be negligible.
> We could replace the vector with a multi-indexed set class or 3-4
ordered set
> structures holding pointers to the same pictures but keeping them in
different
> orders. Worth the complexity for 16-32 elements?

My concern isn't about the run-time cost of sorting but rather with the
code complexity.
I meant it when I asked whether all the sorting is in fact necessary.
Is it? What does the spec require?

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1199
content/common/gpu/media/vaapi_h264_decoder.cc:1199: void
VaapiH264Decoder::ConstructReferencePicListsB(H264SliceHeader*
slice_hdr) {
The impl of this method is super-similar to ConstructReferencePicListsP.
IWBN to have the commonality extracted so the differences are obvious.

(also it would be nice to have similar commenting style; this function
is much more informative than the one above)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1383

content/common/gpu/media/vaapi_h264_decoder.cc:1383: // Shift all
pictures starting from refIdxLX to the right.

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Can you try to extract common functionality into helper functions to
avoid
> > duplicate structures?

> Well, again, this is intentional. This is exactly as spec'ed. Exact
code from
> spec, to ensure it's correct and easily verifiable. When deciding
whether to
> optimize, I always put exactness with the spec before optimized and
modified
> code if the gains looked small or were only to improve readability.
For a person
> looking at spec, they could just compare this verbatim (hence my links
to
> relevant spec chapters too).

"Optimized" is a red-herring; my concern is with code readability.
Similar to the naming situation, I think it's much more important that
chromium code be readable in its own right than that it match a
horribly-unreadable spec.
"Verifiable" has two conflicting definitions: the first (your meaning)
is that it can be compared to the spec to ensure it does what the spec
says. The second is that it can be verified to do reasonable things by
inspection.
I believe optimizing for the latter is more important (the former can
also be verified by watching movies, the latter cannot be verified any
other way).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1419

content/common/gpu/media/vaapi_h264_decoder.cc:1419: // Must be run in
GLXContext thread.

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Replace this comment with a DCHECK at the top of the function.

> I didn't want to pollute this class with threading stuff, that's the
biggest
> reason for separating this class from VAVDA. I DCHECK in the caller,
in VAVDA,
> before calling this function instead. I'd prefer keeping it this way,
it's
> already pretty complicated here.

I implied this elsewhere, but I don't think putting DCHECKs about
MessageLoop::current() & posting client callbacks to a particular loop
complicates this class.
The fact that you need the comment already means this class is paying
the complexity tax. As a concrete example, I'm proposing you replace:

// Must be run in GLXContext thread.

with
DCHECK(ChildThread::current()->message_loop() ==
MessageLoop::current());
What about the latter strikes you as complicating this class?

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1432

content/common/gpu/media/vaapi_h264_decoder.cc:1432: // Put the decoded
data into XPixmap bound to the texture.

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > What is this doing?
> > The comment reads to me like "copy from the VA surface to the
pixmap" but I
> > thought you said this is zero-copy. What's up?

> This is driver/platform-dependent and implemented by it.
> The real 100% no-copy-at-all-evar can only be guaranteed if we are not
passing
> stuff back to GL (not even mentioning SW compositor), but displaying
it directly
> from here with vaapi calls, not GL.

> That said though, since GL and VAAPI is the same Intel driver and same
device
> (non-discrete GPU), it would be a horrible omission if they did a copy
here.

How could they *not* copy? This is the first time VAAPI is hearing that
you want the particular decoded frame on the particular pixmap.
Or are you saying that the actual decode will be delayed until this call
is made? That would be fine.

> Moreover, in SW decoding case it's not really a zero-copy, since when
we write
> to the texture buffer bound to a pixmap with a software decoder, the
data has to
> be uploaded to the GPU.

Of course; but why are you bringing up SW decode?

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1464
content/common/gpu/media/vaapi_h264_decoder.cc:1464: DCHECK(msg_loop_);

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > I thought you were going to PostTask to this msg loop, not DCHECK it
here.

> I'm overgenerous with DCHECKs sometimes, but I have a feeling we think
of them
> in a different way. For me they are noops whenever performance is
cared about at
> all, and that I shouldn't care about performance when they are
present. Am I
> missing something?

DCHECKs are indeed compiled out of release builds.

My point (again) is not about the run-time cost of the DCHECK, but
rather that I expected you to run the callback (the next line) on
msg_loop_, not in the current context.
Perhaps another way to put it is: msg_loop_ is never used for anything
in this class other than this one DCHECK (which could just as well have
been done in Initialize()).

As I said in another comment, changing this line & the next to be:
msg_loop_->PostTask(FROM_HERE, base::Bind(
&Decoder::SyncAndDeliverPicture, dec_surface));
seems like the right thing to do to me.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.cc#newcode1518

content/common/gpu/media/vaapi_h264_decoder.cc:1518: // Prepare
reference picture lists if required (B and S/SP slices).

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > comment is conditional but next two lines aren't.
> Yeah, it refers to the ifs below as well.

Then remove the newline at 1521.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h
File content/common/gpu/media/vaapi_h264_decoder.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode21
content/common/gpu/media/vaapi_h264_decoder.h:21: #include
"base/memory/linked_ptr.h"

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Any reason you didn't use linked_ptr for PicsVector?

> Yes, this:

http://www.corp.google.com/eng/doc/devguide/cpp/primer.html#google_pointers
> (paragraph a bit further down about linked_ptr). "linked_ptr: avoid in
new code"
> and it also mentions plain pointer containers are fine and I've seen
plenty of
> those in Chrome code, so I thought they were a valid alternative.

I was surprised b/c you used linked_ptr elsewhere in this CL.
Probably best to replace that use with IdMap.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode39

content/common/gpu/media/vaapi_h264_decoder.h:39: // using the Texture
From Pixmap GLX extension.

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > is this really a concern of this class?

> Well, binding is related to VASurfaces. I can have a subclass, but
it'd still
> kind of require association of pixmaps with VASurfaces, so I thought
separating
> this wasn't worth adding an additional map of VASurface->"pixmap
related
> things". We already have PoC->dec_surface and
output_buffer_id->dec_surface.

I meant that pixmapness is not part of the API.
Client provides textures and this paints on them; pixmapness is an impl
detail.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode44

content/common/gpu/media/vaapi_h264_decoder.h:44: // is further referred
to as "GLX thread", while decoding thread is referred

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Is there some reason not to refer to this as the ChildThread
(chromium-wide
> > convention & class)?

> Not sure if we are on the same page. GLX thread is the parent thread.

"ChildThread" means the "main" thread of a chromium process.
I.e. the "render" thread in the renderer process, the "gpu" thread in
the GPU process, etc. I think "GLX thread" is the ChildThread in the
GPU process (the main GPU thread).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode74

content/common/gpu/media/vaapi_h264_decoder.h:74: // |msg_loop| is where
the |output_pic_callback| should be posted, with

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Is msg_loop the decoder thread or the child thread?
> > If the latter, then why not use MessageLoop::current() in the impl
of
> Initialize
> > (given you require Initialize must be called on the child thread)?

> Decoder thread is the child thread. The main thread is the GLX thread.
> Initialize is to be called on the main (i.e. GLX thread).

See elsewhere (and replay my original comment)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode119

content/common/gpu/media/vaapi_h264_decoder.h:119: DecResult
DecodeInitial(int32 input_id);

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Why is input_id a param to this & DecodeOneFrame instead of being a
param to
> > SetStream?

> To support having more than one frame in one stream chunk (as we
discussed we
> didn't want to make an assumption we wouldn't).

While a stream chunk may contain multiple NALUs, it only has one
bitstream_buffer_id.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode142

content/common/gpu/media/vaapi_h264_decoder.h:142: enum {
kNumReqPictures = H264DPB::kDPBMaxSize + 6 };

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Isn't this an excessive amount of memory to be holding on to?

> I need to be able to keep at least H264DPB::kDPBMaxSize of frames for
reference
> and delayed output, in other words that many pictures kept in decoder
at once
> and not given back to the client. This is per spec.
> +6 was empirical, based on how much the client held on to before it
actually
> started giving back (although I might have gotten away with +5).

I get that you need to keep that many frames in some form, but does it
need to be decompressed images on textures?
Alternatively, WDYT of allocating them as needed?
Possibly piman@ has thoughts on this subject.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode221

content/common/gpu/media/vaapi_h264_decoder.h:221:
scoped_ptr<H264Picture> curr_pic_;

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Why not a simple member?

> You mean a pointer?

Retract comment (I misunderstood the member's use).

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode237

content/common/gpu/media/vaapi_h264_decoder.h:237: // Values related to
previously decoded reference picture.

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Does it not make sense to have a
> > H264Picture last_ref_picture_;
> > ?

> I was deliberating it for quite some time, but I felt that associated
management
> pain wasn't worth it (not mentioning wasted memory). The last ref
picture
> doesn't have to still be present in DPB or anywhere, so it's not just
a question
> of pointing to something that is in DPB.

A copy wouldn't involve mgmt pain and the wasted memory is in the 10s of
bytes; certainly irrelevant in this space.

> I guess a refcounted ptr would solve this though...

No thanks ;) (refcounting is for when you don't know what the ownership
semantics are or when you know they're quite complicated; here all you
need to do is make a copy where you would otherwise set these members)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode308

content/common/gpu/media/vaapi_h264_decoder.h:308: bool
BindToTexture(int width, int height);

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > inline into ctor and replace bool ret value with setting available
to false,
> > which callers can check post-construction?

> The class is constructed before we can bind, that's why this is
separate and not
> called from the constructor. VASurfaces are allocated in one go in one
driver
> call and a context is associated with all of them. If I were to
construct here,
> I'd have to allocate VASurfaces one by one and then gather them back
and
> associate context with it, which if it'd fail would be harder to
report too. And
> I would have to trigger this somehow after all BindToTexture() calls
are done.
> So I felt this way would be much more clear.

I don't understand the relationship between this comment and the code of
VaapiH264Decoder::AssignPictureBuffer in which I see
new DecodeSurface()
followed by a BindToTexture call, in between which I see no VASurface
allocation or context association.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode317

content/common/gpu/media/vaapi_h264_decoder.h:317: // Initialize/destroy
static state of this class (X Display and current

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Why static? Instead leave ownership w/ decoder object and pass refs
in ctor.
> > Then decoder is in charge of init/destroy.

> Only because it isn't related to decoder

The fact that only the decoder knows when to do this belies this
statement, IMO.
Please move it.
(static is evil; you need to have a very compelling reason to use it.
For example, did you really mean to share this state between different
VDA instances?)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_h264_decoder.h#newcode338

content/common/gpu/media/vaapi_h264_decoder.h:338: typedef std::map<int,
DecodeSurface*> POCToDecodeSurfaces;

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > why not a linked_ptr?

> See my previous comment, but I guess you really prefer it... :)

I was unclear. I don't want linked_ptr. I was confused by why you used
it for one thing but not others.

As long as ownership lies with decode_surfaces_ then it's fine to use a
raw pointer here.
(decode_surfaces_ should be an IdMap that owns its pointees)

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc#newcode128

content/common/gpu/media/vaapi_video_decode_accelerator.cc:128: void
VaapiVideoDecodeAccelerator::RequestPictureBuffers(int num_pics,

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Why not pass the VDA::Client to the decoder, so it can call
> > RequestPictureBuffer, PictureReady, etc directly?
> > (I suspect having these trampolines here is only adding code &
indirection,
> > unnecessarily)
> >
> > Once you do that I think there's some cleanup that can be done (i.e.
decoder's
> > pic_width/height() are no longer necessary, as is
GetRequiredNumOfPics()).

> I was really trying not to add additional complexity to the decoder
class.

I'm saying I think passing Client to the decoder would actually
*simplify* the decoder.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc#newcode442

content/common/gpu/media/vaapi_video_decode_accelerator.cc:442:
base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Wait, wat??
> > decoder tells VAVDA a pic is ready but then VAVDA has to ask decoder
to sync
> the
> > texture? Isn't that crazy? Why wouldn't decoder sync the texture
before
> > telling VAVDA it's ready?

> Decoder can't sync, because it has to be done on GLX thread. I also
want to
> postpone sync as much as possible and not stall the decoder thread for
it, it
> has enough to do anyway.

Then why not pass the decoder the GLX thread and have it sync on it and
then call fire the client callback?

> I also don't want the decoder to bother with threading as I mentioned
before.

I think that ship has sailed; the API for the decoder is explicit about
what can be called on which thread, and it receives a messageloop as a
param.

> So in other words, this lets both the decoder (software parsing, etc)
and HW
> (hardware decode) not have to wait for each other and for the VDA
client.
> Otherwise the three of them, HW, decoder thread and GLX thread would
have to
> converge at one point. Yuck :)

I agree that would be yuck. I don't think my suggestion would cause
this, though.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media/vaapi_video_decode_accelerator.cc#newcode442

content/common/gpu/media/vaapi_video_decode_accelerator.cc:442:
base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,

> Decoder can't sync, because it has to be done on GLX thread. I also
want to
> postpone sync as much as possible and not stall the decoder thread for
it, it
> has enough to do anyway.

Then why not pass the decoder the GLX thread and have it sync on it and
then call fire the client callback?

> I also don't want the decoder to bother with threading as I mentioned
before.

I think that ship has sailed; the API for the decoder is explicit about
what can be called on which thread, and it receives a messageloop as a
param.

I agree that would be yuck. I don't think my suggestion would cause
this, though.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.gypi#newcode386
content/content_common.gypi:386: 'common/gpu/media/h264_dpb.cc',

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > l.386-389 are not cros/x64-specific, are they?
> > (move to the unconditional sources list above, even though they
won't be used
> by
> > non-vaapi-containing builds?)

> Do we want to compile them in if they are not used?

I'm on the fence. Leave them for now I guess.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi
File content/content_tests.gypi (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/content_tests.gypi#newcode493
content/content_tests.gypi:493:
'common/gpu/media/video_decode_accelerator_unittest.cc',

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Were you able to make this test work w/ VAVDA?
> > (IWBN if this CL included a change to this stanza to make that work)

> It requires adding GLX support and is not trivial. But I will
definitely do it.

TODO unless you plan to do it in this CL.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2160

content/renderer/render_view_impl.cc:2160: && (defined(ARCH_CPU_ARMEL)
|| defined(ARCH_CPU_X86_FAMILY))

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > are there any cros platforms *other* than x86 & arm?

> Not that I am aware of. Wanted to be extra safe. Drop it?

Yes please.

http://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_view_impl.cc#newcode2161

content/renderer/render_view_impl.cc:2161: // Currently only cros has
any HW video decode support in

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > delete?

> Your call, was here before :)

You duplicated the pre-existing line.
(note this is a copy of l.2155; this copy should be deleted, the other
one should stay)

http://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium
File third_party/libva/README.chromium (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README.chromium#newcode1
third_party/libva/README.chromium:1: Name: libva

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Is there some reason to copy libva into third_party instead of
pulling it in
> via
> > DEPS?
> >
> > Hint: I think the answer is "no", unless you have local
modifications that
> must
> > be applied to the third-party code that can't be upstreamed.

> We are actually thinking with Stephane about this to optimize the
driver
> ourselves, but not for now at least.

Then please revert the libva part of this CL and add it to DEPS instead
(after asking chrome-infrastructure-team to create a git mirror repo).

http://chromiumcodereview.appspot.com/9814001/diff/1/ui/gfx/gl/gl_context_egl.h
File ui/gfx/gl/gl_context_egl.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/ui/gfx/gl/gl_context_egl.h#newcode36

ui/gfx/gl/gl_context_egl.h:36: virtual EGLDisplay GetDisplay() OVERRIDE;

On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > I think the ui/ part of this CL should be pulled out and be its own
CL (since
> > it's so independent).
> >
> > However, it's not clear to me you need these changes, anyway.
> > In both GLX/EGL the relevant display is a static property, which you
can get
> > with
> > base::MessagePumpForUI::GetDefaultXDisplay();
> > and
> > eglGetDisplay(EGL_DEFAULT_DISPLAY);
> > resp.
> >
> > (also, AFAICT, you only actually use the GLX version, so the EGL
edit is
> > completely unneeded?)

> Yes, it will be the case in the final CL, just didn't want to give you
something
> that wouldn't compile and my understanding was that I can't have
dependencies
> between CLs here, can I?

My CR process doesn't (usually) include compiling the change under
review.
You can have chained CLs; SOP is to mention in the review message that
this CL depends on another CL(s).
(as I mentioned in my last email, it's possible to specify which
diff/baselines you want git cl upload to use)