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

Pawel Osciak

8 years, 9 months ago (2012-03-21 06:57:24 UTC) #1

Ami GONE FROM CHROMIUM

I ran out of code-reviewing steam at vaapi_h264_decoder.cc:1739 (and before looking through VAVDA.{h,cc}). I'll hold ...

8 years, 9 months ago (2012-03-21 13:16:23 UTC) #2

Pawel Osciak

On 2012/03/21 13:16:23, Ami Fischman wrote: > I ran out of code-reviewing steam at vaapi_h264_decoder.cc:1739 ...

8 years, 9 months ago (2012-03-21 18:40:16 UTC) #3

Pawel Osciak

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: ...

8 years, 9 months ago (2012-03-21 18:40:34 UTC) #4

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
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/medi...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
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/medi...
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/medi...
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/medi...
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/medi...
File content/common/gpu/media/h264_parser.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
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 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/medi...
File content/common/gpu/media/vaapi_h264_decoder.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_SUCCESS_OR_ERROR(vares, err_msg)                   \
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".

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:259: // Still initialized and
ready to decode
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/medi...
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/medi...
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/medi...
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/medi...
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 ;)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:613: // Return reference frames
in reverse order of insertion
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:642: poc_to_decode_surfaces_[poc]
= iter->second.get();
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/medi...
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:686:
pic_param.picture_width_in_mbs_minus1 = sps->pic_width_in_mbs_minus1;
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/medi...
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:954: // TODO posciak: start using
vaMapBuffer instead of vaCreateBuffer wherever
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:985: buffers[i] =
pending_va_bufs_.front();
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/medi...
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/medi...
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1057:
DCHECK_EQ(sizeof(curr_pic_->ref_pic_marking),
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1074: switch
(parser_.GetSPS(curr_sps_id_)->pic_order_cnt_type) {
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...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1165: // Below is not correct if
a field.
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1188:
std::sort(RefPicList0_.begin(), RefPicList0_.end(),
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?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1335: -
((int)list_mod->abs_diff_pic_num_minus1 + 1)
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 :)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1383: // Shift all pictures
starting from refIdxLX to the right.
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).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1419: // Must be run in
GLXContext thread.
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1432: // Put the decoded data
into XPixmap bound to the texture.
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1456: // require the data to be
synced with texture contents. Client must use
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/medi...
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?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1518: // Prepare reference
picture lists if required (B and S/SP slices).
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/medi...
File content/common/gpu/media/vaapi_h264_decoder.h (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
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/medi...
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:39: // using the Texture From
Pixmap GLX extension.
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
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 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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:45: // to as "decoder thread". If
not specified otherwise, a function is to be
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
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 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/medi...
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 ;)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:99: bool PutPicToTexture(int32
picture_buffer_id);
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/medi...
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:107: // state, so the playback of
the same stream can be resumed (possibly from
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:119: DecResult DecodeInitial(int32
input_id);
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).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:142: enum { kNumReqPictures =
H264DPB::kDPBMaxSize + 6 };
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/medi...
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?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:237: // Values related to
previously decoded reference picture.
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/medi...
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 :(

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:308: bool BindToTexture(int width,
int height);
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:317: // Initialize/destroy static
state of this class (X Display and current
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:338: typedef std::map<int,
DecodeSurface*> POCToDecodeSurfaces;
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/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:128: void
VaapiVideoDecodeAccelerator::RequestPictureBuffers(int num_pics,
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.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:442:
base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,
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....
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common....
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.g...
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/c...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/content/public/common/c...
content/public/common/content_switches.cc:342: const char
kEnableVaapiDecodeAcceleration[] = "enable-vaapi-decode-accel";
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Can VAAPI be used for anything other than video decode acceleration?
No

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

Ok

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render...
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?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2161: // Currently only cros has any HW
video decode support in
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/READM...
File third_party/libva/README.chromium (right):

https://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/READM...
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/READM...
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_eg...
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?

Ami GONE FROM CHROMIUM

On Wed, Mar 21, 2012 at 7:40 PM, <posciak@chromium.org> wrote: > I'm sorry that parser ...

8 years, 9 months ago (2012-03-22 14:53:10 UTC) #5

On Wed, Mar 21, 2012 at 7:40 PM, <posciak@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/medi...
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).

Ami GONE FROM CHROMIUM

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 ...

8 years, 9 months ago (2012-03-22 17:01:36 UTC) #6

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...
File content/common/gpu/media/h264_dpb.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media...
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...
File content/common/gpu/media/h264_dpb.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media...
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...
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...
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...
File content/common/gpu/media/h264_parser.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media...
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...
File content/common/gpu/media/vaapi_h264_decoder.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media...
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...
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...
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...
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...
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...
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...
content/common/gpu/media/vaapi_h264_decoder.cc:686:
pic_param.picture_width_in_mbs_minus1 = sps->pic_width_in_mbs_minus1;
On 2012/03/21 18:40:35, posciak wrote:
> 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.

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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
File content/common/gpu/media/vaapi_h264_decoder.h (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/media...
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...
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...
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/content_common.gypi
File content/content_common.gypi (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common.g...
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.gy...
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_...
File content/renderer/render_view_impl.cc (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render_...
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_...
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...
File third_party/libva/README.chromium (right):

http://chromiumcodereview.appspot.com/9814001/diff/1/third_party/libva/README...
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...
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)

Pawel Osciak

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#newcode705 content/browser/gpu/gpu_process_host.cc:705: switches::kEnableVaapiDecodeAcceleration, On 2012/03/21 13:16:24, Ami Fischman wrote: > Keep ...

8 years, 8 months ago (2012-04-05 10:37:20 UTC) #7

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:705:
switches::kEnableVaapiDecodeAcceleration,
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Keep sorted?

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:27: #else  // OS_WIN
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Having #else\n#if is confusing; replace with #elif?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:36: #include
"ui/gfx/gl/gl_context.h"
On 2012/03/21 13:16:24, Ami Fischman wrote:
> dup of l.40 below?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:155: DLOG(INFO) <<
"Initializing VAAPI HW decoder.";
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop

Ok, although there is a similar print for DXVA a few lines above...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:159:
(gfx::GLContextGLX*) stub_->decoder()->GetGLContext();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Both this and the cast below should be static_cast<>'s

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.cc:59: PicsVector::iterator iter =
std::find_if(pics_.begin(), pics_.end(),
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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;
> 
> ?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.cc:66: PicsVector::iterator iter =
pics_.begin();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.cc:98: H264Picture*
H264DPB::GetRefPicByPicNum(int pic_num, bool long_term) {
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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*()

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.cc:132: return
*(std::min_element(short_refs.begin(), short_refs.end(),
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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).

Agreed on copying, although this thing is actually more complicated than what
you proposed:

  H264Picture* ret = NULL;
  for (int i = 0; i < pics_.size(); ++i) {
    if (pics_[i]->ref && !pics_[i]->long_term &&
        (!ret || pics_[i]->frame_num_wrap < ret->frame_num_wrap))
      ret = pic;
  }
  return ret;

I'll use a temporary pic pointer though.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:28: int TopFieldOrderCnt;
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:56: // Utility function objects
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)

Done. And got rid of majority of them.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:113: typedef std::vector<H264Picture*>
PicsVector;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Move into H264Picture and and rename to just Vector?

Well, things got quite a bit more verbose with H264Picture:: prefix, but ok.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:133: void StorePic(H264Picture* pic);
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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).

It's just feels dirty to get scoped_ptr and then immediately convert it to
regular pointer and use it as such. Also please see my longer explanation why
I'm not using linked_ptr later on.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:142: H264Picture* GetShortRefPicByPicNum(int
pic_num);
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Assuming this isn't relinquishing ownership, IWBN to return const-ref instead
of
> pointer here (and below).

What do you mean by const-ref? Ref to const type? Can't, I modify the returned
pictures.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:161: // Iterators for direct access to DPB
contents.
On 2012/03/21 13:16:24, Ami Fischman wrote:
> These will be invalidated by mutating methods elsewhere in this class.  Do you
> want to document anything about that?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:174: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra newline

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_SUCCESS_OR_ERROR(vares, err_msg)                   \
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:19: DLOG(ERROR) << (err_msg);    
                          \
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/DLOG/DVLOG/ everywhere unless you have a reason not to.

I thought this kind of failure was reason enough :) But will do.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:20: DLOG(ERROR) << "VA error: "
<< vaapi_vaErrorStr(vares); \
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Can these be a single LOG?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:29: return (ret);                
                          \
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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?

It will change state in higher layers, as this will return false to them and
they will then decide what state to set. Main use of those states is for
internal resource management though, to be able to clean up our internal
resources/state (and keep decoding properly after Reset()).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:37: typedef VADisplay
(*vaapiGetDisplayGLX)(Display *dpy);
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Types need to be Capitalized (here and elsewhere)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:38: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> remove the newlines between these typedefs?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:122: vaapiGetDisplayGLX
vaapi_vaGetDisplayGLX =
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/vaapi_vaGetDisplayGLX/VAAPI_GetDisplayGLX/
> (and similar renaming throughout)

I tried to keep the full vaapi function names in the names (they are in vaFooBar
format) for easier lookup, but don't feel strongly about it.
Ok.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:123:
reinterpret_cast<vaapiGetDisplayGLX>(dlsym(vaapi_glx_handle,
On 2012/03/21 13:16:24, Ami Fischman wrote:
> does using a macro for this make sense?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:187: return
(vaapi_vaGetDisplayGLX
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop the parens?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:254:
num_available_decode_surfaces_ = num_assigned_vaapi_surfaces_;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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()?

Yeah, good idea. Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:259: // Still initialized and
ready to decode
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:263: void
VaapiH264Decoder::Destroy() {
On 2012/03/21 13:16:24, Ami Fischman wrote:
> early-return if state_==kUninitialized ?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:268: switch (state_) {
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Uh... not sure how it got there, thanks for spotting.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:284: default:
On 2012/03/21 13:16:24, Ami Fischman wrote:
> better to drop default: and let the compiler yell at you if you forgot a case.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:292: return
AreVaapiFunctionPointersInitialized();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> This doesn't check HW support, only presence of libraries.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:311: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra \n

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:315: // Has to be run in
GLXContext thread
On 2012/03/21 13:16:24, Ami Fischman wrote:
> replace this comment (and others like it) with DCHECK at the top of the
> function.

This is information for the caller, I don't know which exactly is the GLX
thread. DCHECKs are in the caller. This class can be used without threading.
Comments are for callers, if they wanted to multithread this class. It's similar
to having thread safe or not comments.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:322: VAStatus va_res;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> first use is at l.349; move this there.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:362: DLOG(INFO) << "YUV420 not
supported";
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/DLOG/DVLOG/
> s/INFO/ERROR/

My understanding was *VLOGs were associated with numerical kV, and numbers were
to be used instead of defines like INFO. There are almost no DVLOG([^0-9]) calls
in entire Chrome codebase. There are 3 DVLOG(ERROR) and 0 DVLOG(INFO),
DVLOG(FATAL), etc.
I'll s/DLOG/DVLOG(1) for errors, ok?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:382: {}
On 2012/03/21 13:16:24, Ami Fischman wrote:
> opening brace belongs on previous line

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:394: GLX_Y_INVERTED_EXT, GL_TRUE,
On 2012/03/21 13:16:24, Ami Fischman wrote:
> what's this about?

Asking for a framebuffer config with origin at top-left. Unfortunately, doesn't
really help as shader has flip hardcoded.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:419: static int x11_error = 0;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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...

Well, I was restoring them after use ;)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:473: XSync(x_display_, False);
On 2012/03/21 18:40:35, posciak wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:506: scoped_ptr<DecodeSurface>
dec_surface(new DecodeSurface());
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/scoped/linked/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:508: DCHECK_EQ(state_,
kDecoding);
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)
>

Yes, I ignore it, but I don't see too much of a problem. The surface that
errored out will not be used. VDA::APB is a void anyway. Do we want a
PLATFORM_FAILURE in this case? I don't treat this as an error that prevents us
from decoding. Worst case we may end up with 0 surfaces to use though.
 
> In the future APB will be callable mid-stream to support frame-size changes.

VAVDA is not really ready for this yet. I don't think other VDAs are though
either...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:536:
make_linked_ptr(dec_surface.release()))).second;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> indent is wrong (and make_linked_ptr/release shouldn't be necessary after you
> change dec_surface to be a linked_ptr).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:549: pic_height_,
VA_RT_FORMAT_YUV420,
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:567: VAStatus va_status;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> please standardize on a single name for this throughout the file.
> I think "res" is fine (as opposed to va_res/va_status/etc).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:582:
poc_to_decode_surfaces_[pic->PicOrderCnt]->va_surface_id;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> operator[] will add a NULL pointer if the index is not found.  Ok?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:598: NOTREACHED();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop default: clause.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:613: // Return reference frames
in reverse order of insertion
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:614: for (rit = dpb_.rbegin(), i
= 0; rit != dpb_.rend() && i < 16; ++rit)
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/16/kSomething/

16 is actually hardcoded in libva.h... I should've used arraysize().

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:615: if ((*rit)->ref)
On 2012/03/21 13:16:24, Ami Fischman wrote:
> multi-line for bodies require braces.
> here & elsewhere; please make a pass to make sure this is taken care of for
all
> control structures.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:654: /*DCHECK(it !=
poc_to_decode_surfaces_.end());*/
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:686:
pic_param.picture_width_in_mbs_minus1 = sps->pic_width_in_mbs_minus1;
On 2012/03/22 17:01:36, Ami Fischman wrote:
> On 2012/03/21 18:40:35, posciak wrote:
> > 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.
> 
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:783: for (i = 0; i < 2; ++i)
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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??

VAIQMatrixBufferH264.ScalingList8x8 (the one in vaapi) is of size 2. My scaling
list 8x8 is of size 6 per spec, but how many of those are used (2 or 6) depends
on current chroma format. VAAPI does not support the format with 6.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:985: buffers[i] =
pending_va_bufs_.front();
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

To me it is simpler to have one queue variable I don't have to check for
overflows every time I do something with it than maintaining an array of fixed
size and a separate counter of how many entries are valid in it, updating every
time I do something, and also checking for overflow every time. If you still
feel it'd be simpler than a queue, I'll switch...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1017: frame_decoded_ = true;
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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_/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1031: curr_pic_->field =
slice_hdr->bottom_field_flag ? H264Picture::FIELD_BOTTOM
On 2012/03/21 13:16:24, Ami Fischman wrote:
> multi-line then clause requires braces for then & else clauses.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1037: // Not correct if not a
field.
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1057:
DCHECK_EQ(sizeof(curr_pic_->ref_pic_marking),
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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).

Can't agree more. Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1074: switch
(parser_.GetSPS(curr_sps_id_)->pic_order_cnt_type) {
On 2012/03/21 18:40:35, posciak wrote:
> 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...

Will need to find streams that actually use != 0, seems uncommon actually. So
I'll keep case 0 only.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1123: break;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> break is unnecessary after return (here and elsewhere).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1139: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra newlines are unnecessary.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1148: default:
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop default clause

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1156: H264Picture* pic;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> move to first use at l.1159.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1188:
std::sort(RefPicList0_.begin(), RefPicList0_.end(),
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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?

It is required by spec, why would I be doing it otherwise? :) The comment 4
lines above explains the requirements, need one list of reference pictures, made
up of two sequences:
1. Short term reference pics sorted by PicNum, descending,
2. Followed by long term reference pics sorted by LongTermPicNum, ascending. So
needs two sorts and that's what I'm doing.

I don't see how you could do that without sorting twice (and without a
multi-indexed structure of sorts like I mentioned above, considering that
ref_pic_list1 construction is even more complicated). Aren't you exaggerating a
bit saying "all this sorting"? :)

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1194:
//DCHECK((int)RefPicList0_.size() <=
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1195:
//slice_hdr->num_ref_idx_l0_active_minus1 + 1);
On 2012/03/21 13:16:24, Ami Fischman wrote:
> drop

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1199: void
VaapiH264Decoder::ConstructReferencePicListsB(H264SliceHeader* slice_hdr) {
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)

The similarities are deceiving. I unified commenting style, but if you look,
there practically isn't anything there that could be extracted to a common
function.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1272: if (!pic)
On 2012/03/21 13:16:24, Ami Fischman wrote:
> when can this happen?

Sometimes the stream may request more ref pics than we have available. In that
case (per spec) the additional pictures are to be treated as
non-ref/non-existent. I guard against the possibility of stream asking to
reorder them here.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1383: // Shift all pictures
starting from refIdxLX to the right.
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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).

Of course the code should be doing reasonable things. This is a specific case
though, because the spec does not say exactly "do X then Y then Z", it says
"execute the code listed below to do X" and has a piece of code there. Now that
I extracted a part of it and changed variable names, we can eyeball it and say,
"yeah, looks reasonable". But it's more difficult to tell whether it's doing
exactly what it's supposed to.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1419: // Must be run in
GLXContext thread.
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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?

I just don't want to make it do any threading calls/checks. I don't see any need
for that.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1432: // Put the decoded data
into XPixmap bound to the texture.
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.
> 

This is possible.
But whatever happens, this the kind of misunderstanding I wanted to prevent by
bringing up SW decode below - because people refer to our current SW-based TFP
scheme as zero-copy, since we bind to the texture, although there is still an
upload to it. One could argue where do you draw a line of something being
zero-copy or not, but what I'm saying is, in VAAPI case, perhaps it could be
made  not a "zero-copy" even in that (SW) sense, where there is still traffic
RAM->GPU. If by binding a pixmap to a texture you can tell the GPU to eventually
use GPU texture memory, and VASurface is in GPU memory as well, the transfer
could technically happen entirely in GPU (unless CPU tries to access it). Of
course this would require knowledge whether CPU dirtied it, but if we are
overwriting the texture anyway, we could skip that, potentially at least.
I'm trying to get a hold of some folks at Intel to get answers about that from
them actually.


> > 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?
> 

See above.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1464: DCHECK(msg_loop_);
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

This DCHECK is actually a leftover from when this function was indeed posting a
task. I left it here for a bit longer to make sure everything worked as before,
before removing it.
Again, I intentionally removed everything threading-related from this class. No
need for it knowing anything about msg loops.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1518: // Prepare reference
picture lists if required (B and S/SP slices).
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1528: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> lots of unnecessary \n's IMO

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1530: DVLOG(4) <<
"================ B slice, POC= " << curr_pic_->PicOrderCnt
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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.

Right, those ones can go easily.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1555: struct
MarkLongTermPicUnusedForRefIfOverMax
On 2012/03/21 13:16:24, Ami Fischman wrote:
> ditto my comment about explicit loops over algorithms here and below.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1722: // Shouldn't get here.
On 2012/03/21 13:16:24, Ami Fischman wrote:
> NOTIMPLEMENTED is LOG(ERROR).  Can you replace some usages of it with DCHECKs
> instead?

The code couldn't have really gotten here, unless there was a bug in my code.
OTOH this means DCHECK should be enough anyway.
Although personally I don't understand, if we don't want NOTIMPLEMENTED to error
out, but want to be quiet about it instead, why do we have it that way in the
first place?

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:12: #include "h264_parser.h"
On 2012/03/21 18:40:35, posciak wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:21: #include
"base/memory/linked_ptr.h"
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Than one instance of linked_ptr was actually because I wrote that code after you
pointed out in parser review that I should be using linked_ptr and it felt it
would fit nicely there.

As for the IdMap, I don't need a hash map for this.

I preferred plain pointers in the vector as I'm passing them around quite a bit
and creating temporary containers  containing those pointers and even shuffling
them around in them every frame. I've done some investigation and found plenty
of other places which did that, so I thought it was a valid choice.
But if you have strong feelings about this and feel that even with all this
juggling I do with the pointers (passing around, changing contents by
non-owners, temporary vectors of them in non-owners, reordering those temporary
vectors every frame, etc.), I can reconsider using vector of linked pointers.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:22: #include
"base/memory/ref_counted.h"
On 2012/03/21 13:16:24, Ami Fischman wrote:
> unused?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:24: #include "base/message_loop.h"
On 2012/03/21 13:16:24, Ami Fischman wrote:
> fwd-declare

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:26: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra \n

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:29: // A H264 decoder for use for
VA-API-specific decoding. Provides features not
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/A/An/  (here and elsewhere in comments in this CL)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:39: // using the Texture From
Pixmap GLX extension.
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Ah ok, so you are saying just remove it from the doc, since it's an impl detail?
Ok.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:44: // is further referred to as
"GLX thread", while decoding thread is referred
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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).

Right. The decoder thread is a ChildThread of the GLX thread (more specifically,
a GpuChildThread deriving from ChildThread), which you say is a ChildThread
itself, so I'm using GLX thread and decoder thread to avoid confusion.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:45: // to as "decoder thread". If
not specified otherwise, a function is to be
On 2012/03/21 18:40:35, posciak wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:49: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra newline

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:50: // Callback to be invoked on
the client when a picture
On 2012/03/21 13:16:24, Ami Fischman wrote:
> sentence is cut off

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:54: // Client is expected to call
PutPicToTexture() to when it is ready to
On 2012/03/21 13:16:24, Ami Fischman wrote:
> Please make an editing pass on this paragraph to english it up.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:56: typedef void
(*OutputPicCallbackPtr)(void*, int32, int32);
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:60: kDecError, // error while
decoding
On 2012/03/21 13:16:24, Ami Fischman wrote:
> in-line comments require 2 spaces before the //.
> (here and elsewhere)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:60: kDecError, // error while
decoding
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:61: // TODO posciak: add
unsupported stream error (for now treated as error in
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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?)

We talked about this and decided that falling back gracefully to SW on
unsupported stream is tricky and for now we just want to report a platform
error. But I wanted to keep a trace of this somewhere that perhaps this could be
revisited sometime in the future.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:103: bool Flush();
On 2012/03/21 18:40:35, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > what's the ret value?
> 
> Could've errored out at OutPic. I'll document.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:119: DecResult DecodeInitial(int32
input_id);
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.  

Ok, but I'd prefer to leave it this way. We could want to increment
bitstream_buffer_id after each frame, not on each SetStream().

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:127: // Valid only after a
successful Initialize().
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/Initialize/DecodeInitial/ ?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:128: int pic_height() { return
pic_height_; }
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/pic_// here and below.

decoder->pic_height() feels more informative to me than decoder->height().

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:136: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra \n

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:142: enum { kNumReqPictures =
H264DPB::kDPBMaxSize + 6 };
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

I need to keep decompressed images.
As for keeping textures, I would otherwise have to bind/unbind them on each
frame. This is not a good thing to do, as they are a pretty expensive thing to
do on each frame. Moreover, it would complicate things, as we would have to bind
them on OutputPic callback and keep additional layer of state for running out of
textures (in addition to running out of surfaces) and store pics for output on
an additional queue in VAVDA.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:150: kError, // error in kDecoding
state
On 2012/03/21 13:16:24, Ami Fischman wrote:
> initial caps above please

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:237: // Values related to
previously decoded reference picture.
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)

Ok, so I'm leaving it as is. I think we prefer copying 5 ints over copying a
structure that is hundreds of bytes...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:251: 
On 2012/03/21 13:16:24, Ami Fischman wrote:
> extra \n

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:253: // Vaapi-related functions.
On 2012/03/21 13:16:24, Ami Fischman wrote:
> style guide requires functions before members.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:261: // These queue up data for HW
decoder to be commited on running HW decode.
On 2012/03/21 13:16:24, Ami Fischman wrote:
> committed

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:284: struct DecodeSurface {
On 2012/03/21 18:40:35, posciak wrote:
> 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 :(

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:303: DecodeSurface();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> move these to the top of the class

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:308: bool BindToTexture(int width,
int height);
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Yeah, sorry, too many changes and I was thinking about previous version. This
had been true before I had to reimplement binding after seeing the
implementation in libva was suboptimal for us.
So yeah, fixing according to your comment.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:311: void UnbindFromTexture();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> inline into dtor

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:315: bool Sync();
On 2012/03/21 13:16:24, Ami Fischman wrote:
> not defined?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:317: // Initialize/destroy static
state of this class (X Display and current
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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?)

Yes, that was my intention. FBConfig is framebuffer configuration, it's per X
Screen. They shouldn't change across VDAs.
It's the same with x_display. fb_config is associated with display, so they both
have to be either static or not. But it's not a big deal, just two pointers, so
I'm making them both non-static as you asked.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:325: static
base::LazyInstance<GLXFBConfig> fb_config_;
On 2012/03/21 13:16:24, Ami Fischman wrote:
> I think LazyInstance can be dropped.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:338: typedef std::map<int,
DecodeSurface*> POCToDecodeSurfaces;
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)

I actually almost finished converting to IDMap, but found it annoying to use.
Any advantages over map of linked pointers? I found the semantics annoyingly
different from a std::map, there is no clear() method and iterator usage is
awkward and more verbose than std::map, with awkward GetCurrentValue(). The key
is hardcoded to be int32, which slightly more potentially error-prone when you
rely on your key type being int32 and use AddWithID()...

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:361: // Assing a surface to
PicOrderCount, removing it from the available surfaces
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/Assing/Assign/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:363: // to decode too.
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/too/to/

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:128: void
VaapiVideoDecodeAccelerator::RequestPictureBuffers(int num_pics,
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.  

VAVDA controls logic behind whether it's required to RPB or not. Decoder
supports one operation - Decode (in two versions - initial and one that decodes
until it produces one frame). I see your point and yes, we would have slightly
less code if I merged VAVDA and Decoder, but I still like my design more.
Decoder doesn't concern itself what needs to be done if it runs out of input,
where to get output from and how, etc. It just decodes and provides related
information, like width/height. That feels like a good way to separate things to
me. It's clearer, more readable and easier to understand and maintain.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:442:
base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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'm not passing message_loop or anything related to the decoder anymore. I'd
strongly prefer to keep it this way.

> > 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.  
> 

Yeah, that was terrible, but I got rid of messageloop from it, thanks to your
suggestion to use base::Callback. That was a very happy moment.
API is explicit about what *could* be run concurrently, *if* the caller wanted
to. And whether PostTask or whatever else might be used is a detail the decoder
thankfully no longer has to concern itself with. 

> > 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.

You suggested the decoder synced before telling VAVDA it's ready. And I just
realized you meant the decoder class not decoder thread. Yeah, it could. But
really, VAVDA exists to manage threading, implement VDA interface and do other
VDA-related stuff (like juggling buffers and mapping them), so that decoder
doesn't have to.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/content_common....
content/content_common.gypi:386: 'common/gpu/media/h264_dpb.cc',
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Assuming you meant leave them as is.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2158: // implementations are added, relax
the #if above.
On 2012/03/21 13:16:24, Ami Fischman wrote:
> s/above/below/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2160: && (defined(ARCH_CPU_ARMEL) ||
defined(ARCH_CPU_X86_FAMILY))
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2161: // Currently only cros has any HW
video decode support in
On 2012/03/22 17:01:36, Ami Fischman wrote:
> 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)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2164:
WebGraphicsContext3DCommandBufferImpl* context3d =
On 2012/03/21 13:16:24, Ami Fischman wrote:
> 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)

Do we want to have one common flag then for acceleration on ARM and x86, as well
as on cros and on Linux?

Pawel Osciak

Updated with a fix for occasional decode freeze on output falling behind for more demanding ...

8 years, 8 months ago (2012-04-06 07:27:44 UTC) #8

Ami GONE FROM CHROMIUM

Didn't get through the CL during the weekend. Still need to review decoder.cc and vavda.{h,cc}. ...

8 years, 8 months ago (2012-04-09 02:41:47 UTC) #9

Didn't get through the CL during the weekend.
Still need to review decoder.cc and vavda.{h,cc}.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:113: typedef std::vector<H264Picture*>
PicsVector;
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Move into H264Picture and and rename to just Vector?
> 
> Well, things got quite a bit more verbose with H264Picture:: prefix, but ok.

Umm, should it instead by a typedef inside H264DPB?

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:133: void StorePic(H264Picture* pic);
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/22 17:01:36, Ami Fischman wrote:
> > 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).
> 
> It's just feels dirty to get scoped_ptr and then immediately convert it to
> regular pointer and use it as such.

It doesn't feel dirty to me.  It feels like a highlight that something's wrong. 
Specifically, DPB *owns* the pic so it should store its pointer in a scoped
something.
In this case I think if you used a ScopedVector it would help clarify the
situation.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:142: H264Picture* GetShortRefPicByPicNum(int
pic_num);
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > Assuming this isn't relinquishing ownership, IWBN to return const-ref
instead
> of
> > pointer here (and below).
> 
> What do you mean by const-ref? Ref to const type? Can't, I modify the returned
> pictures.

Oh.
I think you have a mental model that the DPB owns all pictures, and anyone
getting a ptr to a picture from DPB never gets ownership (and must take care not
to use the returned pointer once the DPB is reset/deleted).  IWBN to document
that somewhere.

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:44: // is further referred to as
"GLX thread", while decoding thread is referred
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/22 17:01:36, Ami Fischman wrote:
> > 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).
> 
> Right. The decoder thread is a ChildThread of the GLX thread (more
specifically,
> a GpuChildThread deriving from ChildThread), which you say is a ChildThread
> itself, so I'm using GLX thread and decoder thread to avoid confusion.

No.  The Decoder thread is not "a ChildThread of".  ChildThread is the name of a
class, and that's the sense I'm using it in. 
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/child_threa...
The "child" in the name refers to the _process_ in which the thread runs being a
"child" of the main chrome process.  The thread which is a ChildThread (of which
there is exactly one in a chrome process) is the main thread of that process.

What you are calling "GLX thread" is the main thread of the GPU process, and is
(chrome-) conventionally referred to as the ChildThread in its process. 
Referring to having a  ChildThread and a Decoder thread (which happens to be
_started by_ the ChildThread) should not be confusing.
I suspect you didn't realize ChildThread was a type name and were thinking of
the decoder as being a "child" of the main GPU process' thread b/c the latter
starts the former.  That's not a sense of ChildThread that is used in chromium.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:119: DecResult DecodeInitial(int32
input_id);
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/22 17:01:36, Ami Fischman wrote:
> > 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.  
> 
> Ok, but I'd prefer to leave it this way. We could want to increment
> bitstream_buffer_id after each frame, not on each SetStream().

Doing so would make no sense.  BBID is an identifier that starts at the VDA
client in the renderer.  Deciding to mangle it in the VDA impl in the GPU
process would only lead to serious confusion.  I think you should undo this.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:237: // Values related to
previously decoded reference picture.
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/22 17:01:36, Ami Fischman wrote:
> > 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)
> 
> Ok, so I'm leaving it as is. I think we prefer copying 5 ints over copying a
> structure that is hundreds of bytes...

Not really; copying consecutive O(100) bytes per frame is practically free.
Fine to leave it as-is for now and revisit if it turns out we need more than
these 5.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.h:338: typedef std::map<int,
DecodeSurface*> POCToDecodeSurfaces;
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/22 17:01:36, Ami Fischman wrote:
> > 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)
> 
> I actually almost finished converting to IDMap, but found it annoying to use.
> Any advantages over map of linked pointers? I found the semantics annoyingly
> different from a std::map, there is no clear() method and iterator usage is
> awkward and more verbose than std::map, with awkward GetCurrentValue(). The
key
> is hardcoded to be int32, which slightly more potentially error-prone when you
> rely on your key type being int32 and use AddWithID()...

Do what you feel results in most-readable code.  I would ignore the linked_ptr
warning you found in the corp guidelines; linked_ptr is a fine choice for
single-thread, few-references pointer storage in STL containers.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/browser/gpu...
content/browser/gpu/gpu_process_host.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
CL description still needs update.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:27: #elif
defined(OS_CHROMEOS)
else isn't buying you clarity here; I'd close the OS_WIN above and open a new
#if CROS here.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:30: #elif
defined(ARCH_CPU_X86_FAMILY)
Is it the case that all cros/intel have VA HW support?  
Is it harmless to build this into platforms that don't have it?

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:38: DVLOG(1) << "Missing POC: " << poc;
Is this not a DCHECK-able offense?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:45: RemovePic(it);
Isn't this a bug (erase inside RemovePic invalidates |it|).
I think you need to lose the ++it in the loop and instead
if (condition)
  RemovePic(it++);
else
  ++it;

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:77: DVLOG(1) << "Missing short ref pic num:
" << pic_num;
Here and elsewhere aren't these properties of a malformed stream?
I ask b/c things like ShiftRightAndInsert DCHECK the non-NULLness of them.
DCHECK==programming invariant.  If it's invalid for these to return NULL why not
just CHECK here?
If a malformed stream can trigger a NULL return then callers must test for
NULLness and error out instead of crashing the process.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:93: H264Picture* ret = NULL;
A trick to avoid the need for the !ret (and an extra set of parens below) is to
initialize ret to pics_[0], assuming you know pics_ is non-empty.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:112: IsH264PictureOutputted());
Why algorithms instead of a loop?
(here and below).

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.h:57: content::H264DecRefPicMarking
s/content::// here and on the next line (this entire file is in the content
namespace)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.h:126: H264Picture::PtrVector pics_;
I think this wants to be a ScopedVector<H264Picture>, instead.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:192: void Get(int32 input_id, int
poc);
"Get" with void ret val and no output params is strange.
Replace Get/Put with Acquire/Release?

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:26: namespace content {
This means you can drop content:: from everywhere below.
(please make a pass over the entire CL for this)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:39: // called on the display
thread (i.e. the thread that has the GLX context
Per my comments on the previous PS, I'd refer to the ChildThread (a.k.a. main
thread of the GPU process) and the Decoder thread (a.k.a. the thread started by
VAVDA).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:51: typedef
base::Callback<void(int32, int32)> OutputPicCallback;
nit: FWIW new-style callbacks (which this is) are referred to using FooCB and
foo_cb_ in media code.  
(b/c the old-style callbacks used FooCallback and foo_callback_ and it was handy
during the migration to have a different convention for the new type to tell
what was what, and now that the migration is over we're enjoying the shorter
type & variable names).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:54: enum DecResult {
s/DecResult/DecodeResult/
but isn't this really DecoderState?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:98: // Returns false if any of the
resulting invocations of the callback fail.
I suspect this is in the realm of DCHECK (i.e. programming errors or platform
errors).  Does this really need to support returning a bool?
As support for my skepticism I present the evidence that the (only) caller of
this function does not inspect its return value.

Generally coding an API whose callers must inspect a return value to look for
errors is bug-prone, if that return value is not otherwise inspected.  IOW if
you're returning a Something* then it's easy to use NULL as an error condition,
and callers will be looking at that ret value anyway b/c the "happy" codepath
must use it, too.  But if a function is otherwise void then it's really easy for
callers to forget to inspect the return value.  In these cases it's much more
useful if the API instead has an out-of-band error-reporting mechanism, such as
an OnError callback where all error handling can be centralized.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:129: static size_t
GetRequiredNumOfPictures();
Why size_t (an unsigned type)?
In practice we're talking about values in the range of 2-10.  Introducing the
complexity of unsigned arithmetic for this is unwise.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:137: enum { kNumReqPictures =
H264DPB::kDPBMaxSize + 6 };
Please replace the "6" with the explicit math.
I believe you want 
kDPBMaxSize + media::limits::kMaxVideoFrames
(previously I had a +1 for your "an additional one for the one currently being
decoded" but now I think that's part of the m::l:kMVF number, no?)

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/public/comm...
content/public/common/content_switches.cc:347: // Enables hardware acceleration
for video decode on Linux and ChromeOS
VAAPI code is not built on linux so this comment over-promises.
Should the gyp file be relaxed?

Ami GONE FROM CHROMIUM

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#newcode362 content/common/gpu/media/vaapi_h264_decoder.cc:362: DLOG(INFO) << "YUV420 not supported"; On 2012/04/05 10:37:20, posciak ...

8 years, 8 months ago (2012-04-09 21:35:53 UTC) #10

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

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:362: DLOG(INFO) << "YUV420 not
supported";
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > s/DLOG/DVLOG/
> > s/INFO/ERROR/
> 
> My understanding was *VLOGs were associated with numerical kV, and numbers
were
> to be used instead of defines like INFO. There are almost no DVLOG([^0-9])
calls
> in entire Chrome codebase. There are 3 DVLOG(ERROR) and 0 DVLOG(INFO),
> DVLOG(FATAL), etc.
> I'll s/DLOG/DVLOG(1) for errors, ok?

Sorry, my comment was cryptic.  Indeed, *VLOG must take a numeric argument (I
bet all the examples you found are bugs that are only compiling b/c ERROR
happens to be a numeric define).
It's fine to DLOG(ERROR) for errors.
DLOG(INFO) is almost never the right thing; instead use DVLOG(1).  This is b/c
if errors aren't happening, you don't want to spam the console output of
chromium devs not working on this specific code, which are certainly the vast
majority.

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:985: buffers[i] =
pending_va_bufs_.front();
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/22 17:01:36, Ami Fischman wrote:
> > 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.
> 
> To me it is simpler to have one queue variable I don't have to check for
> overflows every time I do something with it than maintaining an array of fixed
> size and a separate counter of how many entries are valid in it, updating
every
> time I do something, and also checking for overflow every time. If you still
> feel it'd be simpler than a queue, I'll switch...

Would we both be happier if pending_{slice,va}_bufs_ were
std::vector<VABufferId>'s?  That'd let you push_back without worry of overflow,
get automatic tracking of counts, and still let you pass the underlying storage
(without having to have the loops to copy it) directly to VA (as
&pending_va_bufs[0]).

https://chromiumcodereview.appspot.com/9814001/diff/1/content/common/gpu/medi...
content/common/gpu/media/vaapi_h264_decoder.cc:1722: // Shouldn't get here.
On 2012/04/05 10:37:20, posciak wrote:
> On 2012/03/21 13:16:24, Ami Fischman wrote:
> > NOTIMPLEMENTED is LOG(ERROR).  Can you replace some usages of it with
DCHECKs
> > instead?
> 
> The code couldn't have really gotten here, unless there was a bug in my code.
> OTOH this means DCHECK should be enough anyway.
> Although personally I don't understand, if we don't want NOTIMPLEMENTED to
error
> out, but want to be quiet about it instead, why do we have it that way in the
> first place?

NOTIMPLEMENTED is a placeholder for things that need to be implemented before a
feature is complete (although of course it's been applied unevenly throughout
the codebase).

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_LOG_ON_ERROR(vares, err_msg)                     \
in the macro defs you use "vares" but then everywhere else in this file you use
"va_res".  va_res is more chromiumy (since they're two "words").

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_LOG_ON_ERROR(vares, err_msg)                     \
I question not returning in this case.
The 4 call-sites where you use this macro are all destroy/shutdown-related.  But
it's not at all clear to me that if one VA call fails, it's reasonable to
continue making more VA calls.  Is this stuff documented in the API?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:19: DVLOG(1) << (err_msg)        
                        \
The parens around err_msg are ill-advised.  For example they prevent callers
from being able to << stream different types (i.e. requires them to pass
fully-formed strings).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:24: #define
VA_SUCCESS_OR_RETURN(vares, err_msg, ret)           \
Every single invocation of this macro uses a |ret| value of false.  Just sayin'.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:27: DVLOG(1) << (err_msg)        
                        \
This could as well call VA_LOG_ON_ERROR to avoid dup'ing the logging code.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:108: Vaapi##name VAAPI_##name =  
                                 \
indent

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:132: && VAAPI_DisplayIsValid
chrome-style puts operators on preceding line

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:507: bool
VaapiH264Decoder::AssignPictureBuffer(int32 picture_buffer_id,
Another example of a method returning bool to signal error, but whose callers
don't inspect the return value.
This code would be much less bug-prone if you transitioned the decoder to error
state in cases like this.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:517: linked_ptr<DecodeSurface>
dec_surface(new DecodeSurface(fb_config_,
move fb_config_ to the next line

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:586: // Cannot provide a ref
picture, will corrupt output, but may be able
Does this indicate an invalid stream?

(ffmpeg has a long/large history of security holes caused by attempting to
continue decoding in the face of errors; I would much rather not decode 0.0001%
of streams than open a security hole)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:679: DCHECK(pps);
This means "it is a programming error if pps is NULL here" (i.e. even a corrupt
stream shouldn't cause this to happen).
In that case why even support returning NULL from GetPPS?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:690: #define FromSPSToPP2(a, b)
pic_param.b = sps->a;
Needs to be named like FROM_SPS_TO_PP2()
but maybe better as PP2_FROM_SPS()

(ditto for others below)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:695:
FromSPSToPP(bit_depth_chroma_minus8);
Please #undef'ine macros when you're done w/ them, for macros like this that are
only used in a short block.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:705:
pic_param.seq_fields.bits.MinLumaBiPredSize8x8 = (sps->level_idc >= 31);
gut-check: should that really be >=31 or >31 (or >=32)?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:941: // TODO posciak: start using
vaMapBuffer instead of vaCreateBuffer wherever
TODO(posciak)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:1152: struct PicNumDescCompare {
I'm not as smooth w/ STL as I might be, but I thought it also works to:
static bool PicNumDescCompare(
    const H264Picture* a, const H264Picture* b) const {
  return a->pic_num > b->pic_num;
}

(without the need for the wrapping struct/operator())

No?

(here and elsewhere)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:1416: if (!it->second->Sync())
if (!foo) 
  return false;
return true;

is equivalent to
return foo;

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:38:
decoder_thread_.Stop();
This will drain the thread's messageloop & wait for the thread to exit,
synchronously.  Did you mean to do that?
In particular, the decoder thread could be waiting for a new input and never
return here.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:47: :
input_ready_(true, false),  // manually Reset() and initially not signalled
indent is off (and for the rest of the initializers below, too).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:63: bool res = true;
decl at first use, below.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:67: res =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableVaapi);
ISTM this would make more sense at a higher layer.
Either GVDA.cc, or even better yet, before reaching the GPU process in RVI
(http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi...)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:70: goto done;
seriously?  goto?

Are you just testing me for being awake? :)

Note that when Initialize returns false it *shouldn't* call NotifyInitializeDone
(instead, GVDA will call NotifyError on its behalf).  So you shouldn't need the
goto at all.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:75:
base::Unretained(this)));
Unretained is usually the wrong thing to do.  (see my comment in the .h file; I
think you want an unadorned "this" here)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:86:
MessageLoop::current()->PostTask(FROM_HERE,
you mean to say:
message_loop_->PostTask(FROM_HERE, base::Bind(
    &Client::NotifyInitializeDone, client_);

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:88: return res;
s/res/true/ assuming you early-return false on failure.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:91: void
VaapiVideoDecodeAccelerator::NotifyInitializeDone() {
drop

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:97: void
VaapiVideoDecodeAccelerator::SetGlxState(Display* x_display,
I'm not sure there's a reason to make this a separate setter, instead of making
these params of the ctor.
(I know this mimics OVDA::SetEglState(), but I can't remember why I had that
that way or if it was unintentional bogosity).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:117: if
(!decoder_.PutPicToTexture(output_id)) {
Use the macro?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:146: // If current
buffer is still set, return it to the client.
It seems strange to me that you only notify the client that an input has been
fully decoded when the decoder asks for the *next* chunk.  My guess is that this
could lead to livelock at EOS (without markers in the stream) b/c the decoder is
ready for more work, but the client knows it has nothing more to feed the
decoder and is just waiting for news that it is done with the last buffer to
send a flush.
Can you instead notify the client when the buffer is actually consumed?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:155: // If there is
no more input buffers ready, return. We will come back
s/is/are/

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:167: // Decoder is
waiting on |input_ready_|, so we are safe to do this.
This is some very fiddly logic!  You are effectively using the combination of
WaitableEvent and the fact that *this* method must run on the render thread as a
guarantee of mutual exclusion.  

Please see the "However" warning on
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/waita...
I think you'd be better off with a Lock+ConditionVariable here.

For example, I wonder if this is correct in the presence of Reset() and
already-posted TGNIB tasks.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:168:
decoder_.SetStream((uint8*)curr_input_buffer_->shm->memory(),
static_cast

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:184:
CHECK(shm.get());
You never need to CHECK the result of a new in chromium.  A failure to allocate
will terminate the process.
(here and elsewhere)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:247:
decoder_thread_.message_loop()->PostTask(FROM_HERE,
?? we're already on the decoder's thread.  Why post another task?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:250: break;
This method might become slightly less confusing if you s/break/return/ from
here down (to emph. that there's on common code that executes after each of the
switch cases).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:265: default:
drop default cases everywhere in this CL unless they're needed (let compiler
warn you of missing cases).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:279:
decoder_thread_.message_loop()->PostTask(FROM_HERE,
ditto already on decoder thread

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:326: // the decoder
thread is not running yet, so we can safely do this.
Initialize() calls decoder_thread_.Start(), and Initialize must complete before
AssignPictureBuffers is called (in fact, Decode() must have been called before
APB is called, or else the decoder wouldn't know what dimension pictures to ask
for).

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:333:
decoder_thread_.message_loop()->PostTask(FROM_HERE,
Why is this the right thing to do?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:342:
decoder_thread_.message_loop()->PostTask(FROM_HERE,
already on the decoder thread.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:343:
base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
Why is this right?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:359:
decoder_.Flush();
This only emits already-decoded pictures, but the contract of VDA::Flush() is
that it NotifyFlushDone()s only when all inputs are done decoding, which can
include inputs queued in VAVDA here.

Generally the fact that this impl is synchronous should be a red flag.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:360:
decoder_.Reset();
Why is Reset() necessary?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:365: WaitForInput();
why?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:398:
//WaitForInput();
??

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:407: resetting_ =
true;
this should go before the above call to avoid racing.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:409: // decoding
threads will exit immediately due to resetting_ set to true.
s/threads/tasks/

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:426: // No decoder
tasks running anymore, so safe to destroy.
decoder_ is also accessed on the main thread, not just on the decoder thread,
and you don't have any guards to prevent already-queued tasks from attempting to
use a Destroy()ed decoder.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:13: #include <map>
Unused.
(please make a pass on the CL to verify headers are only included where
necessary)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:36:
NON_EXPORTED_BASE(public base::NonThreadSafe) {
I don't think you understand what NonThreadSafe is for :)
(you never call CalledOnValidThread(), and this class actually *does* expect to
get called on multiple threads).
Drop this base.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:39:
VaapiVideoDecodeAccelerator(media::VideoDecodeAccelerator::Client* client);
s/media::VideoDecodeAccelerator::// I think (since this is already a VDA).  
This also applies elsewhere in this file and in the .cc file.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:42: bool
Initialize(media::VideoCodecProfile profile) OVERRIDE;
missing "virtual" keyword from many methods.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:57: // Notify the
client that initialization is finished (successful or not).
See my comment in the .cc file, but I think you can drop all these methods
(through StopOnError) that just wrap client_ calls by binding to the client call
in the first place.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:78: // queue it for
use.
s/use/decode/

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:101: // Used by the
decoder thread to request more input stream.
s/stream/streams/

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:120: // Current input
buffer set in decoder.
s/set in/at/

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:127: 
In your quest to keep threading concerns out of the decoder, you have the result
that both VAVDA & the decoder have data members that are accessed on exactly one
thread, and some that are accessed on both, but none of this is documented. 
While the decoder gains the benefit of theoretically being runnable in an
unthreaded environment without cognitive dissonance in the comments, it pays in
being very difficult to verify that no races exist.  IMO this is very
unfortunate.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:149: static void
OutputPicCallback(VaapiVideoDecodeAccelerator* vavda,
It's silly to use a static method and pass |this| as its first param.
Just drop "static" and the first param and bind to (not Unretained!) this.

Pawel Osciak

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/browser/gpu/gpu_process_host.cc File content/browser/gpu/gpu_process_host.cc (right): https://chromiumcodereview.appspot.com/9814001/diff/14001/content/browser/gpu/gpu_process_host.cc#newcode1 content/browser/gpu/gpu_process_host.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...

8 years, 7 months ago (2012-05-03 16:22:07 UTC) #11

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/browser/gpu...
content/browser/gpu/gpu_process_host.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/09 02:41:47, Ami Fischman wrote:
> CL description still needs update.

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:27: #elif
defined(OS_CHROMEOS)
On 2012/04/09 02:41:47, Ami Fischman wrote:
> else isn't buying you clarity here; I'd close the OS_WIN above and open a new
> #if CROS here.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/gpu_video_decode_accelerator.cc:30: #elif
defined(ARCH_CPU_X86_FAMILY)
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Is it the case that all cros/intel have VA HW support?  
> Is it harmless to build this into platforms that don't have it?
No, not all of them, but it's harmless, libva will report HW decode is not
supported and VAVDA::Initialize() will fail.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:38: DVLOG(1) << "Missing POC: " << poc;
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Is this not a DCHECK-able offense?

Yeah, I'm reconsidering error resilience especially with what you've said about
not caring about malformed streams too much.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:45: RemovePic(it);
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Isn't this a bug (erase inside RemovePic invalidates |it|).
> I think you need to lose the ++it in the loop and instead
> if (condition)
>   RemovePic(it++);
> else
>   ++it;

Of course. I always forget.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:77: DVLOG(1) << "Missing short ref pic num:
" << pic_num;
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Here and elsewhere aren't these properties of a malformed stream?
> I ask b/c things like ShiftRightAndInsert DCHECK the non-NULLness of them.
> DCHECK==programming invariant.  If it's invalid for these to return NULL why
not
> just CHECK here?
> If a malformed stream can trigger a NULL return then callers must test for
> NULLness and error out instead of crashing the process.

My intention was to die gracefully with a stream error, but I failed to do that
in this case. Fixing.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:93: H264Picture* ret = NULL;
On 2012/04/09 02:41:47, Ami Fischman wrote:
> A trick to avoid the need for the !ret (and an extra set of parens below) is
to
> initialize ret to pics_[0], assuming you know pics_ is non-empty.

Yeah, that's basically the case, but feel more comfortable keeping it this
way...

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.cc:112: IsH264PictureOutputted());
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Why algorithms instead of a loop?
> (here and below).

You really hate stl, don't you? ;)
Done, even without iterators, as you like it :)

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.h:57: content::H264DecRefPicMarking
On 2012/04/09 02:41:47, Ami Fischman wrote:
> s/content::// here and on the next line (this entire file is in the content
> namespace)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/h264_dpb.h:126: H264Picture::PtrVector pics_;
On 2012/04/09 02:41:47, Ami Fischman wrote:
> I think this wants to be a ScopedVector<H264Picture>, instead.

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_LOG_ON_ERROR(vares, err_msg)                     \
On 2012/04/09 21:35:53, Ami Fischman wrote:
> I question not returning in this case.
> The 4 call-sites where you use this macro are all destroy/shutdown-related. 
But
> it's not at all clear to me that if one VA call fails, it's reasonable to
> continue making more VA calls.  Is this stuff documented in the API?

Of course not :) VAAPI has no documentation whatsoever, just the header file,
which doesn't say anything about that. So I arbitrarily chose not to return and
continue calling other destroy functions.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:16: #define
VA_LOG_ON_ERROR(vares, err_msg)                     \
On 2012/04/09 21:35:53, Ami Fischman wrote:
> in the macro defs you use "vares" but then everywhere else in this file you
use
> "va_res".  va_res is more chromiumy (since they're two "words").

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:19: DVLOG(1) << (err_msg)        
                        \
On 2012/04/09 21:35:53, Ami Fischman wrote:
> The parens around err_msg are ill-advised.  For example they prevent callers
> from being able to << stream different types (i.e. requires them to pass
> fully-formed strings).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:24: #define
VA_SUCCESS_OR_RETURN(vares, err_msg, ret)           \
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Every single invocation of this macro uses a |ret| value of false.  Just
sayin'.

Yeah, there used to be calls returning void. I feel I'd have to rename it if it
was to always return false. I don't have a preference, leaving as is though.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:27: DVLOG(1) << (err_msg)        
                        \
On 2012/04/09 21:35:53, Ami Fischman wrote:
> This could as well call VA_LOG_ON_ERROR to avoid dup'ing the logging code.

That would require rechecking va_res anyway, so it's just a dup of DVLOG.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:108: Vaapi##name VAAPI_##name =  
                                 \
On 2012/04/09 21:35:53, Ami Fischman wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:132: && VAAPI_DisplayIsValid
On 2012/04/09 21:35:53, Ami Fischman wrote:
> chrome-style puts operators on preceding line

Sigh, making stuff less readable, but oh well...

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:192: void Get(int32 input_id, int
poc);
On 2012/04/09 02:41:47, Ami Fischman wrote:
> "Get" with void ret val and no output params is strange.
> Replace Get/Put with Acquire/Release?

get/put is quite common for resource acquisition, but maybe not in Chrome.
Release stinks a bit of releasing resources, which this doesn't do... Do you
have a strong preference?

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:507: bool
VaapiH264Decoder::AssignPictureBuffer(int32 picture_buffer_id,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Another example of a method returning bool to signal error, but whose callers
> don't inspect the return value.
> This code would be much less bug-prone if you transitioned the decoder to
error
> state in cases like this.

And again because the VDA API doesn't allow returning an error. But this is not
a critical error, we can skip a picture buffer if it fails to bind and still
accept and use others later, so I don't want to transition to an error state
here.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:517: linked_ptr<DecodeSurface>
dec_surface(new DecodeSurface(fb_config_,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> move fb_config_ to the next line

Done, not sure why you prefer it that way though.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:586: // Cannot provide a ref
picture, will corrupt output, but may be able
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Does this indicate an invalid stream?
> 
> (ffmpeg has a long/large history of security holes caused by attempting to
> continue decoding in the face of errors; I would much rather not decode
0.0001%
> of streams than open a security hole)

This will just mess up the decoded pictures for a while, basically not giving
the decoder all data needed to decode, but is not a security hole unless the HW
decoder has a bug.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:679: DCHECK(pps);
On 2012/04/09 21:35:53, Ami Fischman wrote:
> This means "it is a programming error if pps is NULL here" (i.e. even a
corrupt
> stream shouldn't cause this to happen).

Correct, but only because of how the decoder uses the parser.

> In that case why even support returning NULL from GetPPS?

I intended the parser to be self-contained and if there are ever other users,
this could be useful. I would prefer to keep it this way.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:690: #define FromSPSToPP2(a, b)
pic_param.b = sps->a;
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Needs to be named like FROM_SPS_TO_PP2()
> but maybe better as PP2_FROM_SPS()
> 
> (ditto for others below)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:695:
FromSPSToPP(bit_depth_chroma_minus8);
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Please #undef'ine macros when you're done w/ them, for macros like this that
are
> only used in a short block.

Wow, now you are really nitpicking... :) It just lowers readability and I don't
think it really matters in .cc, but since you insist.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:705:
pic_param.seq_fields.bits.MinLumaBiPredSize8x8 = (sps->level_idc >= 31);
On 2012/04/09 21:35:53, Ami Fischman wrote:
> gut-check: should that really be >=31 or >31 (or >=32)?

 >= 31. Per spec.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:941: // TODO posciak: start using
vaMapBuffer instead of vaCreateBuffer wherever
On 2012/04/09 21:35:53, Ami Fischman wrote:
> TODO(posciak)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:1152: struct PicNumDescCompare {
On 2012/04/09 21:35:53, Ami Fischman wrote:
> I'm not as smooth w/ STL as I might be, but I thought it also works to:
> static bool PicNumDescCompare(
>     const H264Picture* a, const H264Picture* b) const {
>   return a->pic_num > b->pic_num;
> }
> 
> (without the need for the wrapping struct/operator())
> 
> No?
> 
> (here and elsewhere)

It can be easier for the compiler to optimize out (usually inline) a member
function like this than a function pointer. At least in theory.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:1416: if (!it->second->Sync())
On 2012/04/09 21:35:53, Ami Fischman wrote:
> if (!foo) 
>   return false;
> return true;
> 
> is equivalent to
> return foo;

Of course.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:26: namespace content {
On 2012/04/09 02:41:47, Ami Fischman wrote:
> This means you can drop content:: from everywhere below.
> (please make a pass over the entire CL for this)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:39: // called on the display
thread (i.e. the thread that has the GLX context
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Per my comments on the previous PS, I'd refer to the ChildThread (a.k.a. main
> thread of the GPU process) and the Decoder thread (a.k.a. the thread started
by
> VAVDA).

Can't emphasize enough how awkward that is for me, but I give up :)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:51: typedef
base::Callback<void(int32, int32)> OutputPicCallback;
On 2012/04/09 02:41:47, Ami Fischman wrote:
> nit: FWIW new-style callbacks (which this is) are referred to using FooCB and
> foo_cb_ in media code.  
> (b/c the old-style callbacks used FooCallback and foo_callback_ and it was
handy
> during the migration to have a different convention for the new type to tell
> what was what, and now that the migration is over we're enjoying the shorter
> type & variable names).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:54: enum DecResult {
On 2012/04/09 02:41:47, Ami Fischman wrote:
> s/DecResult/DecodeResult/
> but isn't this really DecoderState?

No, it's the result. The enum for State is further below.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:98: // Returns false if any of the
resulting invocations of the callback fail.
On 2012/04/09 02:41:47, Ami Fischman wrote:
> I suspect this is in the realm of DCHECK (i.e. programming errors or platform
> errors).  Does this really need to support returning a bool?
> As support for my skepticism I present the evidence that the (only) caller of
> this function does not inspect its return value.
> 

There are two callers:
1. VAVDA on Flush request from up the stack, which indeed doesn't check for the
return value because the VDA interface does not care (VDA::Flush is void).
2. Flush in VaapiH264Decoder::StartNewFrame(), which checks and handles it.

> Generally coding an API whose callers must inspect a return value to look for
> errors is bug-prone, if that return value is not otherwise inspected.  IOW if
> you're returning a Something* then it's easy to use NULL as an error
condition,
> and callers will be looking at that ret value anyway b/c the "happy" codepath
> must use it, too.  But if a function is otherwise void then it's really easy
for
> callers to forget to inspect the return value.  In these cases it's much more
> useful if the API instead has an out-of-band error-reporting mechanism, such
as
> an OnError callback where all error handling can be centralized.

Right now I have an API that returns DecResult with a detailed error. The
internal functions return bool. I feel uncomfortable with replacing internal
error checks with an internal callback, is that what you are suggesting? I would
much prefer adding WARN_UNUSED_RESULT.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:129: static size_t
GetRequiredNumOfPictures();
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Why size_t (an unsigned type)?
> In practice we're talking about values in the range of 2-10.  Introducing the
> complexity of unsigned arithmetic for this is unwise.

Mostly because I'm comparing the return value against size() of a vector in one
case...

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:137: enum { kNumReqPictures =
H264DPB::kDPBMaxSize + 6 };
On 2012/04/09 02:41:47, Ami Fischman wrote:
> Please replace the "6" with the explicit math.
> I believe you want 
> kDPBMaxSize + media::limits::kMaxVideoFrames
> (previously I had a +1 for your "an additional one for the one currently being
> decoded" but now I think that's part of the m::l:kMVF number, no?)

It'll have to be kMVF + 1, as one is also "in flight" in the decoder.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
As the whole threading is rewritten with the next CL, I'm not addressing all
comments.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:47: :
input_ready_(true, false),  // manually Reset() and initially not signalled
On 2012/04/09 21:35:53, Ami Fischman wrote:
> indent is off (and for the rest of the initializers below, too).

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:63: bool res = true;
On 2012/04/09 21:35:53, Ami Fischman wrote:
> decl at first use, below.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:67: res =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableVaapi);
On 2012/04/09 21:35:53, Ami Fischman wrote:
> ISTM this would make more sense at a higher layer.
> Either GVDA.cc, or even better yet, before reaching the GPU process in RVI
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi...)

I'll make a TODO out of it.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:70: goto done;
On 2012/04/09 21:35:53, Ami Fischman wrote:
> seriously?  goto?
> 
> Are you just testing me for being awake? :)

Well, it's not forbidden by the code style :P

> Note that when Initialize returns false it *shouldn't* call
NotifyInitializeDone
> (instead, GVDA will call NotifyError on its behalf).  So you shouldn't need
the
> goto at all.

That was my understanding as well. But if I don't do that and just return false,
it will crash with sometimes different errors (some races are obviously going
on) instead of falling back to SW decode. I'd been struggling with this about a
month ago and the only reliable way I found was this. But now I revisited
briefly and it looks like while just returning false still crashes the process,
calling NotifyError from VAVDA seems to be enough, although I'm pretty sure I
was having problems even with that last time, otherwise I wouldn't have settled
with NotifyInitializeDone.


I wrote the above before syncing to ToT again, and now it stopped working in
both cases (just returning false or returning false + Notify). Wasn't able to
figure out why yet...

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:75:
base::Unretained(this)));
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Unretained is usually the wrong thing to do.  (see my comment in the .h file;
I
> think you want an unadorned "this" here)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:86:
MessageLoop::current()->PostTask(FROM_HERE,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> you mean to say:
> message_loop_->PostTask(FROM_HERE, base::Bind(
>     &Client::NotifyInitializeDone, client_);

As we discussed, we'd have to do Unretained(client_), which could have at least
theoretically been unsafe.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:88: return res;
On 2012/04/09 21:35:53, Ami Fischman wrote:
> s/res/true/ assuming you early-return false on failure.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:97: void
VaapiVideoDecodeAccelerator::SetGlxState(Display* x_display,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> I'm not sure there's a reason to make this a separate setter, instead of
making
> these params of the ctor.
> (I know this mimics OVDA::SetEglState(), but I can't remember why I had that
> that way or if it was unintentional bogosity).

I have no preference. I just made it symmetrical to the existing ones. I think
it could be removed along with others.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:117: if
(!decoder_.PutPicToTexture(output_id)) {
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Use the macro?

Yep, and now we have use for macro not returning false.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:146: // If current
buffer is still set, return it to the client.
On 2012/04/09 21:35:53, Ami Fischman wrote:
> It seems strange to me that you only notify the client that an input has been
> fully decoded when the decoder asks for the *next* chunk.  My guess is that
this
> could lead to livelock at EOS (without markers in the stream) b/c the decoder
is
> ready for more work, but the client knows it has nothing more to feed the
> decoder and is just waiting for news that it is done with the last buffer to
> send a flush.
> Can you instead notify the client when the buffer is actually consumed?

As we discussed on the call, this is not the case. The decoder will keep
decoding until it reaches the end of the buffer, return kNeedMoreStreamData and
VAVDA will then signal NotifyInputBufferRead immediately, without any additional
VDA::Decode() calls from the client.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:168:
decoder_.SetStream((uint8*)curr_input_buffer_->shm->memory(),
On 2012/04/09 21:35:53, Ami Fischman wrote:
> static_cast

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:184:
CHECK(shm.get());
On 2012/04/09 21:35:53, Ami Fischman wrote:
> You never need to CHECK the result of a new in chromium.  A failure to
allocate
> will terminate the process.
> (here and elsewhere)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:184:
CHECK(shm.get());
On 2012/04/09 21:35:53, Ami Fischman wrote:
> You never need to CHECK the result of a new in chromium.  A failure to
allocate
> will terminate the process.
> (here and elsewhere)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:247:
decoder_thread_.message_loop()->PostTask(FROM_HERE,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> ?? we're already on the decoder's thread.  Why post another task?

The design was one task per frame. But not relevant anymore.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:265: default:
On 2012/04/09 21:35:53, Ami Fischman wrote:
> drop default cases everywhere in this CL unless they're needed (let compiler
> warn you of missing cases).

Actually, this one is needed. If the decoder returned kDecodedFrame or
kNoOutputAvailable it would've been a bug.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:279:
decoder_thread_.message_loop()->PostTask(FROM_HERE,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> ditto already on decoder thread

This time, kReadyToDecode would've been a bug.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:13: #include <map>
On 2012/04/09 21:35:53, Ami Fischman wrote:
> Unused.
> (please make a pass on the CL to verify headers are only included where
> necessary)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:36:
NON_EXPORTED_BASE(public base::NonThreadSafe) {
On 2012/04/09 21:35:53, Ami Fischman wrote:
> I don't think you understand what NonThreadSafe is for :)
> (you never call CalledOnValidThread(), and this class actually *does* expect
to
> get called on multiple threads).
> Drop this base.

Kind of :) I used to use CalledOnValidThread() though, but not anymore
obviously.
Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:39:
VaapiVideoDecodeAccelerator(media::VideoDecodeAccelerator::Client* client);
On 2012/04/09 21:35:53, Ami Fischman wrote:
> s/media::VideoDecodeAccelerator::// I think (since this is already a VDA).  
> This also applies elsewhere in this file and in the .cc file.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:42: bool
Initialize(media::VideoCodecProfile profile) OVERRIDE;
On 2012/04/09 21:35:53, Ami Fischman wrote:
> missing "virtual" keyword from many methods.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:57: // Notify the
client that initialization is finished (successful or not).
On 2012/04/09 21:35:53, Ami Fischman wrote:
> See my comment in the .cc file, but I think you can drop all these methods
> (through StopOnError) that just wrap client_ calls by binding to the client
call
> in the first place.

As discussed, I can't do that after all...

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:78: // queue it for
use.
On 2012/04/09 21:35:53, Ami Fischman wrote:
> s/use/decode/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:101: // Used by the
decoder thread to request more input stream.
On 2012/04/09 21:35:53, Ami Fischman wrote:
> s/stream/streams/

I chose "more of the input stream" instead.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:120: // Current input
buffer set in decoder.
On 2012/04/09 21:35:53, Ami Fischman wrote:
> s/set in/at/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:127: 
On 2012/04/09 21:35:53, Ami Fischman wrote:
> In your quest to keep threading concerns out of the decoder, you have the
result
> that both VAVDA & the decoder have data members that are accessed on exactly
one
> thread, and some that are accessed on both, but none of this is documented. 
> While the decoder gains the benefit of theoretically being runnable in an
> unthreaded environment without cognitive dissonance in the comments, it pays
in
> being very difficult to verify that no races exist.  IMO this is very
> unfortunate.

Well, this is still contained to VAVDA, and does not propagate to decoder, so I
still think I succeeded in my quest :)

But this only happens when switching to a new input buffer, I have to
synchronize to access input queue and swap the buffer currently being used in
the decoder. That's basically the only place this happens when both threads are
running.

The only other place - only one thread is running at that time though - is
setting up picture buffers before the decoder thread is started, I have to do
this that way because we have to do this on GLX thread (texture binding).

Both cases are documented in .cc.

I thought I actually made it as minimal as possible and enough to be
understandable.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:149: static void
OutputPicCallback(VaapiVideoDecodeAccelerator* vavda,
On 2012/04/09 21:35:53, Ami Fischman wrote:
> It's silly to use a static method and pass |this| as its first param.
> Just drop "static" and the first param and bind to (not Unretained!) this.

True, I'm not very much used to the Bind-style programming and thus this legacy
of the non-Bind() version.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/public/comm...
content/public/common/content_switches.cc:347: // Enables hardware acceleration
for video decode on Linux and ChromeOS
On 2012/04/09 02:41:47, Ami Fischman wrote:
> VAAPI code is not built on linux so this comment over-promises.
> Should the gyp file be relaxed?

Yes, I want to do this once this gets in and I get to test on a Linux box.

Ami GONE FROM CHROMIUM

There are still a lot of comments here but I think they're mostly little things ...

8 years, 7 months ago (2012-05-03 23:22:53 UTC) #12

Pawel Osciak

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/media/vaapi_h264_decoder.cc File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/media/vaapi_h264_decoder.cc#newcode192 content/common/gpu/media/vaapi_h264_decoder.cc:192: void Get(int32 input_id, int poc); On 2012/05/03 23:22:53, Ami ...

8 years, 7 months ago (2012-05-06 17:49:19 UTC) #13

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:192: void Get(int32 input_id, int
poc);
On 2012/05/03 23:22:53, Ami Fischman wrote:
> On 2012/05/03 16:22:07, posciak wrote:
> > On 2012/04/09 02:41:47, Ami Fischman wrote:
> > > "Get" with void ret val and no output params is strange.
> > > Replace Get/Put with Acquire/Release?
> > 
> > get/put is quite common for resource acquisition, but maybe not in Chrome.
> > Release stinks a bit of releasing resources, which this doesn't do... Do you
> > have a strong preference?
> 
> But Put() *is* releasing a resource!
> MarkUsed/MarkUnused if you have a problem with Release specifically.
> Get/Put are a terrible name-pair IMO.
> 
> 

ok, acquire/release it is :)

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:507: bool
VaapiH264Decoder::AssignPictureBuffer(int32 picture_buffer_id,
On 2012/05/03 23:22:53, Ami Fischman wrote:
> On 2012/05/03 16:22:07, posciak wrote:
> > On 2012/04/09 21:35:53, Ami Fischman wrote:
> > > Another example of a method returning bool to signal error, but whose
> callers
> > > don't inspect the return value.
> > > This code would be much less bug-prone if you transitioned the decoder to
> > error
> > > state in cases like this.
> > 
> > And again because the VDA API doesn't allow returning an error.
> 
> Sure it does - NotifyError().
> 
> > But this is not
> > a critical error, we can skip a picture buffer if it fails to bind and still
> > accept and use others later
> 
> I don't think so.  Doing so will effectively drop the texture on the floor
> (client thinks decoder has it, decoder doesn't remember it), and I bet will
> cause an apparent hang in VRB as it fails to preroll sufficiently many frames.
> 

Ok, will NotifyError() instead.

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:1152: struct PicNumDescCompare {
On 2012/05/03 23:22:53, Ami Fischman wrote:
> On 2012/05/03 16:22:07, posciak wrote:
> > On 2012/04/09 21:35:53, Ami Fischman wrote:
> > > I'm not as smooth w/ STL as I might be, but I thought it also works to:
> > > static bool PicNumDescCompare(
> > >     const H264Picture* a, const H264Picture* b) const {
> > >   return a->pic_num > b->pic_num;
> > > }
> > > 
> > > (without the need for the wrapping struct/operator())
> > > 
> > > No?
> > > 
> > > (here and elsewhere)
> > 
> > It can be easier for the compiler to optimize out (usually inline) a member
> > function like this than a function pointer. At least in theory.
> 
> You've just given the classical justification for obfuscating readability of
> code to enable a theoretical performance improvement (in a place where
> performance is not known to matter).  Dude.

Ok. I'm leaving it this way though, any C++ programmer is used to seeing
function objects and those are as simple as they can get.

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

https://chromiumcodereview.appspot.com/9814001/diff/14001/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:98: // Returns false if any of the
resulting invocations of the callback fail.
On 2012/05/03 23:22:53, Ami Fischman wrote:
> On 2012/05/03 16:22:07, posciak wrote:
> > On 2012/04/09 02:41:47, Ami Fischman wrote:
> > > I suspect this is in the realm of DCHECK (i.e. programming errors or
> platform
> > > errors).  Does this really need to support returning a bool?
> > > As support for my skepticism I present the evidence that the (only) caller
> of
> > > this function does not inspect its return value.
> > > 
> > 
> > There are two callers:
> > 1. VAVDA on Flush request from up the stack, which indeed doesn't check for
> the
> > return value because the VDA interface does not care (VDA::Flush is void).
> 
> Of course the VDA interface *does* care; the fact that VDA::Flush() is void is
a
> consequence of it being an async interface.  The return value of
> decoder->Flush() needs to be inspected and NotifyError() in case of failure. 
> Added comment there.
> 
> > 2. Flush in VaapiH264Decoder::StartNewFrame(), which checks and handles it.
> 
> I missed this one, somehow.
> 
> > Right now I have an API that returns DecResult with a detailed error. The
> > internal functions return bool. I feel uncomfortable with replacing internal
> > error checks with an internal callback, is that what you are suggesting? I
> would
> > much prefer adding WARN_UNUSED_RESULT.
> 
> Ok.  W_U_R is probably worth doing.  Sadly, it isn't as awesome on most (all?)
> platforms as you'd like it to be (there are lots of violations it can't
detect).

Right. Although in this case, ignoring it does not break things, so I was
leaving the choice up to the client.

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

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:19: DVLOG(1) << err_msg          
                       \
On 2012/05/03 23:22:53, Ami Fischman wrote:
> trailing space

Done (wasn't there).

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:354: ++it;
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Comment this must be done before the UnassignSurfaceFromPoC call below b/c
that
> call invalidates |it|?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:358:
++num_available_decode_surfaces_;
On 2012/05/03 23:22:53, Ami Fischman wrote:
> IWBN if ++num_available_decode_surfaces_ could be part of Put() (and ditto
> --n_a_d_s_ part of Get()) but Get/Put are methods on DecodeSurface, not
Decoder.
>  Oh Well.

Yeah, I tried, but didn't see a way around this :(

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.cc:1341: case 1: {
On 2012/05/03 23:22:53, Ami Fischman wrote:
> If you don't like the fact that you had to add the {}'s here and in case 2
> below, you could pull out the 
> H264Picture* pic;
> declaration to be above the switch and avoid it.
> I don't care either way (both options have slight smelliness to them, but I
> don't know of a way to avoid it).

Yeah, didn't like the brace either. Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_h264_decoder.h:137:
media::limits::kMaxVideoFrames + 2 };
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Why 2 and not 1?  (comment needs to match)

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:25: Destroy();       
                                            \
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Does this want to:
> DCHECK_EQ(message_loop_, MessageLoop::current());
> ?

The functions will re-post themselves as needed.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:34: if (message_loop_
!= MessageLoop::current()) {
On 2012/05/03 23:22:53, Ami Fischman wrote:
> I think this can never be true, since the only caller is the R_A_N_O_F()
macro,
> and that calls Destroy(), and that requires being on message_loop_?

True, Destroy() should be re-posting itself.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:40: DVLOG(1) <<
"Stopping on error " << error;
On 2012/05/03 23:22:53, Ami Fischman wrote:
> S/Stopping on/Notifying of/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:48: // manually
Reset() and initially not signalled
On 2012/05/03 23:22:53, Ami Fischman wrote:
> drop comment (belongs to old WE impl)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:66: DCHECK(client_);
On 2012/05/03 23:22:53, Ami Fischman wrote:
> move to ctor

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:77:
base::Bind(&VaapiVideoDecodeAccelerator::OutputPicCallback, this));
On 2012/05/03 23:22:53, Ami Fischman wrote:
> indent style is inconsistent with first arg above.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:88:
&VaapiVideoDecodeAccelerator::NotifyInitializeDone, this));
On 2012/05/03 23:22:53, Ami Fischman wrote:
> GVDA is in charge of calling NotifyInitializeDone() (if this method returns
> true) or NotifyError() (if this method returns false).  You shouldn't have to
> post this task here, and you should be able to outright delete
> VAVDA::NotifyInitializeDone() below.
> 
> (you can confirm by adding a log stmt to GVD::NotifyInitializeDone and
ensuring
> it's only called once per <video> tag).

As per chat, this is required, at least for now.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:99: void
VaapiVideoDecodeAccelerator::SetGlxState(Display* x_display,
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Add TODO to move these to be ctor params?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:110: DCHECK(client_);
On 2012/05/03 23:22:53, Ami Fischman wrote:
> DCHECK() means it is a programming invariant that the condition is always true
> (independent of user data).
> Following a DCHECK() with an if() means one of them is wrong or spurious.
> In this case, client_ can be NULL if an error happened and then an input
buffer
> was drained (possible due to delayed delivery of tasks in one thread vs. the
> other?), so I think the DCHECK should be removed.

Yeah, debugging artifact.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:128: DCHECK(client_);
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Ditto remove the DCHECK.  
> Please make a pass over the CL looking for these DCHECK+if pairs.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:184:
RETURN_AND_NOTIFY_ON_FAILURE(false, "Unexpected result from decoder",
On 2012/05/03 23:22:53, Ami Fischman wrote:
> s/decoder",/decoder: " << res,/
> will also log the unexpected result code.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:200:
input_ready_.Wait();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> multi-line while/if/for statement requires braces around body.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:209: << " size: " <<
(int)curr_input_buffer_->size;
On 2012/05/03 23:22:53, Ami Fischman wrote:
> (int) unnecessary
> (and if it was, it should be using static_cast<int>())

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:226: bool
VaapiVideoDecodeAccelerator::GetOutputBuffers() {
On 2012/05/03 23:22:53, Ami Fischman wrote:
> This method doesn't call RequestPictureBuffers.  Would it be more accurately
> named WaitForReusedPictureBuffers()?

GetInputBuffer() above doesn't request them either... I was considering the same
thing, but settled with this name to emphasize that its result is getting
input/output buffers already set up in decoder, regardless of whether it waited
or not.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:247:
VaapiH264Decoder::DecResult res;
On 2012/05/03 23:22:53, Ami Fischman wrote:
> declare at first use below?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:285: "Unexpected
result from the decoder", PLATFORM_FAILURE, );
On 2012/05/03 23:22:53, Ami Fischman wrote:
> inconsistent indent style

Now I know :)

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:285: "Unexpected
result from the decoder", PLATFORM_FAILURE, );
On 2012/05/03 23:22:53, Ami Fischman wrote:
> s/",/" << res,/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:303:
client_->ProvidePictureBuffers(num_pics, gfx::Size(width, height));
On 2012/05/03 23:22:53, Ami Fischman wrote:
> FWIW, passing a gfx::Size instead of two ints is less error-prone, so I'd
> recommend doing it to this method, as well.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:312: NOTREACHED() <<
"Invalid state";
On 2012/05/03 23:22:53, Ami Fischman wrote:
> RETURN_AND_NOTIFY_ON_FAILURE ?

Well, this would be a a bug, so I thought NOTREACHED() i.e. DCHECK() was better.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:326: switch (state_)
{
On 2012/05/03 23:22:53, Ami Fischman wrote:
> This is reading state_ without holding the lock_.
> (in general, if you can do a run under tsan that'd be nice)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:347: return;
On 2012/05/03 23:22:53, Ami Fischman wrote:
> silently?  shouldn't this complain?

This means we are shutting down/destroying or otherwise not interested in
running. But a good idea to have a DVLOG here.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:363:
buffers[i].texture_id());
On 2012/05/03 23:22:53, Ami Fischman wrote:
> inconsistent indent.

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:364:
RETURN_AND_NOTIFY_ON_FAILURE(res, "Failed assigning picture buffer",
On 2012/05/03 23:22:53, Ami Fischman wrote:
> could also << the buffer's id & the texture's id (and the index i)

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:389:
decoder_.Flush();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> If this fails shouldn't this NotifyError instead of silently continuing and
> claiming NotifyFlushDone() later?

Good idea.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:389:
decoder_.Flush();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> I think you missed this comment from my previous review:
> 
> This only emits already-decoded pictures, but the contract of VDA::Flush() is
> that it NotifyFlushDone()s only when all inputs are done decoding, which can
> include inputs queued in VAVDA here.
> 
> Generally the fact that this impl is synchronous should be a red flag.

This task is posted by Reset, so it gets queued after any existing decode task.
Reset sets state to kFlushing when doing that. The Decode task will keep
decoding the stream buffers already on input queue, but will never wait and once
it runs out, it will return and the ResetTask will execute cleaning up.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:392:
decoder_.Reset();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Should Decoder::Flush always call its own Reset()?

No, there are cases when it flushes itself without resetting (IDR frames).

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:409:
output_ready_.Signal();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Both this pair of Signal()'s and the corresponding pair in Reset() below are
> "the right thing to do" because of the state_ assignment above.  It would be
> much clearer to put the Signal()'s right below the state_=kFlushing line (and
> kResetting, below).

I actually wanted to emphasize state being changed before posting the task, but
yeah, not really clear and doesn't really do anything.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:418: return;  // We
could've gotten destroyed already.
On 2012/05/03 23:22:53, Ami Fischman wrote:
> DCHECK that?

This is an intentional early exit in case we get a Destroy() before we are done
with flushing. My understanding is it can happen and is not a bug, and we don't
NotifyFlushDone then.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:431: // All the
decoding tasks from before the reset request from client are done
On 2012/05/03 23:22:53, Ami Fischman wrote:
> english up this comment

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:438:
ReturnCurrInputBuffer();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> indent -2

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:465: return;  // We
could've gotten destroyed already.
On 2012/05/03 23:22:53, Ami Fischman wrote:
> DCHECK that

This is an intentional early exit, if we get a destroy request during reset we
just want to get out immediately.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:490: if (state_ ==
kUninitialized || state_ == kDestroying)
On 2012/05/03 23:22:53, Ami Fischman wrote:
> This reads state_ not under lock_.
> Move the auto_lock at l.495 to above this.
> Please make a pass to ensure state_ &friends are never read without lock_
held.

All the places had been properly checked :)
This is intentional, so that RETURN_AND_NOTIFY_ON_FAILURE would work from
anywhere. Note that state_ is only changed on the child thread, so it can't
change here (there is small exception of DestroyTask, but it switches from
kDestroying to kUninitialized and if it's running that means we've been
destroy()ed already and not expecting any other calls).
I didn't want to have a separate function on child thread to just do that change
transition, but it'll be much clearer I guess, so I'll add one.

Note that I don't really have to take the lock on child thread for the state
transitions due to the above (with the assumption of atomicity of assignments to
state of course), but I do anyway.

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

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:35: : public
media::VideoDecodeAccelerator {
On 2012/05/03 23:22:53, Ami Fischman wrote:
> fits on one line now?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:66: // initially
parsed the stream is ready to decode. If the pictures have
On 2012/05/03 23:22:53, Ami Fischman wrote:
> s/is/and is/ ?

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:88: void
ReturnCurrInputBuffer();
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Any reason not to stay consistent with the VDA::Client terminology of
> NotifyEndOfBitstreamBuffer?

It not only notifies, but cleans up as well (mapping) and nulls out
curr_input_buffer_.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:99: // or when
playback is to be resumed from a different location in the stream.
On 2012/05/03 23:22:53, Ami Fischman wrote:
> s/from a different location in the stream/following a seek./

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:108: // decoding task
finishes decoding pending inputs. Makes the decoder flush
On 2012/05/03 23:22:53, Ami Fischman wrote:
> s/decoder flush/decoder return/

Done.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:143: // Initialize()
succeeded, no initial decode and no pictures requested.
On 2012/05/03 23:22:53, Ami Fischman wrote:
> What state is used if Initialize() failed?

kUninitialized. Added comment about this.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:161: // An input
buffer awaiting consumption, provided by the client.
On 2012/05/03 23:22:53, Ami Fischman wrote:
> enums, types, and typedefs belong at the top of the public/protected/private
> section according to style guide.
> We usually make an exception when a name is used only for a single variable
> (typedef'ing right before a member declaration, for example).

Ok, moving it one member down above its only usage.

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/common/gpu/...
content/common/gpu/media/vaapi_video_decode_accelerator.h:186: // Main thread's
message loop
On 2012/05/03 23:22:53, Ami Fischman wrote:
> s/Main thread/ChildThread/ ;)

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/content_com...
content/content_common.gypi:392: '../media/media.gyp:media',
On 2012/05/03 23:22:53, Ami Fischman wrote:
> Unnecessary

Done.

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

https://chromiumcodereview.appspot.com/9814001/diff/23002/content/public/comm...
content/public/common/content_switches.cc:339: // Enables hardware acceleration
for video decode on Linux and ChromeOS
On 2012/05/03 23:22:53, Ami Fischman wrote:
> This says "Linux and ChromeOS" but the .gypi says only on chromeos.  Should
that
> conditional be opened to include linux on x86/x64?

Need to test on Linux. I'll remove Linux from here.

Ami GONE FROM CHROMIUM

https://chromiumcodereview.appspot.com/9814001/diff/36002/content/common/gpu/media/vaapi_h264_decoder.cc File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/9814001/diff/36002/content/common/gpu/media/vaapi_h264_decoder.cc#newcode859 content/common/gpu/media/vaapi_h264_decoder.cc:859: && pps->weighted_pred_flag) op goes on previous line https://chromiumcodereview.appspot.com/9814001/diff/36002/content/common/gpu/media/vaapi_h264_decoder.cc#newcode860 content/common/gpu/media/vaapi_h264_decoder.cc:860: ...

8 years, 7 months ago (2012-05-07 16:38:05 UTC) #14

Pawel Osciak

https://chromiumcodereview.appspot.com/9814001/diff/36002/content/common/gpu/media/vaapi_h264_decoder.cc File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/9814001/diff/36002/content/common/gpu/media/vaapi_h264_decoder.cc#newcode859 content/common/gpu/media/vaapi_h264_decoder.cc:859: && pps->weighted_pred_flag) On 2012/05/07 16:38:05, Ami Fischman wrote: > ...

8 years, 7 months ago (2012-05-07 17:58:00 UTC) #15

Ami GONE FROM CHROMIUM

LGTM brettw: could you please also OWNERS-review this CL?

8 years, 7 months ago (2012-05-07 18:14:11 UTC) #17

brettw

Mostly style nits. http://codereview.chromium.org/9814001/diff/49002/content/common/gpu/media/h264_dpb.cc File content/common/gpu/media/h264_dpb.cc (right): http://codereview.chromium.org/9814001/diff/49002/content/common/gpu/media/h264_dpb.cc#newcode5 content/common/gpu/media/h264_dpb.cc:5: #include "h264_dpb.h" Full include path. http://codereview.chromium.org/9814001/diff/49002/content/common/gpu/media/h264_dpb.h ...

8 years, 7 months ago (2012-05-07 20:34:10 UTC) #18

Pawel Osciak

https://chromiumcodereview.appspot.com/9814001/diff/49002/content/common/gpu/media/h264_dpb.cc File content/common/gpu/media/h264_dpb.cc (right): https://chromiumcodereview.appspot.com/9814001/diff/49002/content/common/gpu/media/h264_dpb.cc#newcode5 content/common/gpu/media/h264_dpb.cc:5: #include "h264_dpb.h" On 2012/05/07 20:34:10, brettw wrote: > Full ...

8 years, 7 months ago (2012-05-07 20:49:55 UTC) #19

piman

https://chromiumcodereview.appspot.com/9814001/diff/49003/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/49003/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode156 content/common/gpu/media/gpu_video_decode_accelerator.cc:156: static_cast<gfx::GLContextGLX*>(stub_->decoder()->GetGLContext()); drive-by: you're assuming x86 == GLX, which may ...

8 years, 7 months ago (2012-05-10 21:55:21 UTC) #21