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

content/common/gpu/media/vaapi_h264_decoder.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// This file contains an implementation of a class that provides H264 decode
+// support for use with VAAPI hardware video decode acceleration on Intel
+// systems.
+
+#ifndef CONTENT_COMMON_GPU_MEDIA_VAAPI_H264_DECODER_H_
+#define CONTENT_COMMON_GPU_MEDIA_VAAPI_H264_DECODER_H_
+
+#include "h264_parser.h"

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

these includes are unstylish (belong in order below, with
content/common/gpu/media/ prefix)

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+#include "h264_dpb.h"
+
+#include <vector>
+#include <queue>
+
+#include <GL/glx.h>
+
+#include "base/lazy_instance.h"
+#include "base/memory/linked_ptr.h"

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Any reason you didn't use linked_ptr for PicsVector?

[Pawel Osciak, 2012/03/21 18:40:35]

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.

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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

[Pawel Osciak, 2012/04/05 10:37:20]

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.

+#include "base/memory/ref_counted.h"

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

unused?

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

fwd-declare

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+#include "media/base/video_decoder_config.h"
+

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

extra \n

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+#include "third_party/libva/va/va.h"
+
+// A H264 decoder for use for VA-API-specific decoding. Provides features not

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/A/An/  (here and elsewhere in comments in this CL)

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+// supported by libva, including stream parsing, reference picture management
+// and other operations not supported by the HW codec.
+//
+// Provides functionality to allow plugging VAAPI HW acceleration into the
+// VDA framework.
+//
+// Clients of this class are expected to pass H264 Annex-B byte stream and
+// will receive decoded pictures via client-provided |OutputPicCallbackPtr|.
+// Supports zero-copy output to client-provided textures, which are bound
+// using the Texture From Pixmap GLX extension.

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

is this really a concern of this class?

[Pawel Osciak, 2012/03/21 18:40:35]

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.

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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

[Pawel Osciak, 2012/04/05 10:37:20]

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

+//
+// If used for multi-threaded decoding, some of the functions have to be
+// called on the display thread (i.e. the thread that has the GLX context
+// passed to Initialize() set as current), as specified below. This thread
+// is further referred to as "GLX thread", while decoding thread is referred

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Is there some reason not to refer to this as the ChildThread (chromium-wide
convention & class)?

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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

[Pawel Osciak, 2012/04/05 10:37:20]

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.

[Ami GONE FROM CHROMIUM, 2012/04/09 02:41:47]

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.

+// to as "decoder thread". If not specified otherwise, a function is to be

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

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

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+// called on the decoder thread.
+class VaapiH264Decoder {
+ public:
+

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

extra newline

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  // Callback to be invoked on the client when a picture

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

sentence is cut off

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  // Arguments: client-private pointer (provided by client in Initialize()),
+  // input buffer id, output buffer id (both provided by the client at the time
+  // of Decode() and AssignPictureBuffer() calls).
+  // Client is expected to call PutPicToTexture() to when it is ready to

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Please make an editing pass on this paragraph to english it up.

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  // output to sync the contents.
+  typedef void (*OutputPicCallbackPtr)(void*, int32, int32);

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

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)

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+
+  // Decode result codes.
+  enum DecResult {
+    kDecError, // error while decoding

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

in-line comments require 2 spaces before the //.
(here and elsewhere)

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

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.

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+    // TODO posciak: add unsupported stream error (for now treated as error in

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

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

[Pawel Osciak, 2012/04/05 10:37:20]

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.

+    // decoding).
+    //KStreamError, // error in stream

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/KS/kS/
(also missing space before k)

+    kReadyToDecode, // successfully initialized
+    kDecodedFrame, // successfully decoded a frame
+    kNeedMoreStreamData, // need more stream data to decode the next frame
+    kNoOutputAvailable, // waiting for the client to free up output surfaces
+  };
+
+  VaapiH264Decoder();
+  ~VaapiH264Decoder();
+
+  // Initializes and sets up libva connection and GL/X11 resources.
+  // |msg_loop| is where the |output_pic_callback| should be posted, with

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

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

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

See elsewhere (and replay my original comment)

+  // |arg| as its first argument.
+  // Must be called on the GLX thread with decoder thread not yet running.
+  bool Initialize(media::VideoCodecProfile profile,
+                  Display* x_display,
+                  GLXContext glx_context,
+                  MessageLoop* msg_loop,
+                  OutputPicCallbackPtr output_pic_callback,
+                  void* arg);
+  void Destroy();
+
+  // TODO reorganize private/public

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Yes, please.  (it's hard to review this class without being sure what its API
is)

[Pawel Osciak, 2012/03/21 18:40:35]

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

+
+  // Notify the decoder that this output buffer has been consumed and
+  // can be reused (overwritten).
+  // Must be run on the decoder thread.
+  void ReusePictureBuffer(int32 picture_buffer_id);
+
+  // Give a new picture buffer (texture) to decoder for use.
+  // Must be run on the GLX thread with decoder thread not yet running.
+  bool AssignPictureBuffer(int32 picture_buffer_id, uint32 texture_id);
+
+  // Sync the data so that the texture for given |picture_buffer_id| can
+  // be displayed.
+  // Must be run on the GLX thread.
+  bool PutPicToTexture(int32 picture_buffer_id);

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

See comment elsewhere, but hopefully this can be inlined into OutputPic() or
whatever ends up calling the picture-ready callback.

[Pawel Osciak, 2012/03/21 18:40:35]

I'll respond in the other place.

+
+  // Have the decoder flush its state and trigger output of all previously
+  // decoded pictures via OutputPicCallback.
+  bool Flush();

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

what's the ret value?

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+
+  // Called while decoding.
+  // Stop decoding, discarding all remaining input/output, but do not flush
+  // state, so the playback of the same stream can be resumed (possibly from

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

What state needs to be kept across Reset()?  I am surprised that any state is
useful at that point.

[Pawel Osciak, 2012/03/21 18:40:35]

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.

+  // another location).
+  void Reset();
+
+  // Set current stream data pointer to |ptr| and |size|.
+  // Must be run on decoder thread.
+  void SetStream(uint8* ptr, size_t size);
+
+  // Start parsing stream to detect picture sizes. Does not produce any
+  // decoded pictures and can be called without providing output textures.
+  // Also to be used after Reset() to find a suitable location in the
+  // stream to resume playback from.
+  DecResult DecodeInitial(int32 input_id);

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Any reason not to make this part of SetStream?

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Why is input_id a param to this & DecodeOneFrame instead of being a param to
SetStream?

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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

[Pawel Osciak, 2012/04/05 10:37:20]

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

[Ami GONE FROM CHROMIUM, 2012/04/09 02:41:47]

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.

+
+  // Runs until a frame is decoded or end of provided stream data buffer
+  // is reached. Decoded pictures will be returned asynchronously via
+  // OutputPicCallback.
+  DecResult DecodeOneFrame(int32 input_id);
+
+  // Return dimensions for output buffer (texture) allocation.
+  // Valid only after a successful Initialize().

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/Initialize/DecodeInitial/ ?

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  int pic_height() { return pic_height_; }

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/pic_// here and below.

[Pawel Osciak, 2012/04/05 10:37:20]

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

+  int pic_width() { return pic_width_; }
+
+  // Return the number of output pictures/textures required for decoding.
+  // Valid after a successful Initialize().
+  static int GetRequiredNumOfPictures();
+
+ private:
+

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

extra \n

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  // We need to keep at least kDPBMaxSize pictures in DPB for
+  // reference/to display later and an additional one for the one currently
+  // being decoded. We also ask for some additional ones since VDA needs
+  // to accumulate a few ready-to-output pictures before it actually starts
+  // displaying and giving them back.
+  enum { kNumReqPictures = H264DPB::kDPBMaxSize + 6 };

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/6/1 + media::limits::kMaxVideoFrames/

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Isn't this an excessive amount of memory to be holding on to?

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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.

[Pawel Osciak, 2012/04/05 10:37:20]

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.

+
+  // Internal state of the decoder.
+  enum State {
+    kUninitialized, // Initialize() not yet called
+    kInitialized, // Initialize() called, pictures requested
+    kDecoding, // DecodeInitial() successful, output surfaces allocated
+    kAfterReset, // after Reset() during decoding
+    kError, // error in kDecoding state

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

initial caps above please

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  };
+
+  // Verify that libva has been loaded successfully and provides required
+  // functions.
+  bool HasHWSupport();
+
+  State state_;
+
+  // A frame has been decoded as the result of the last DecodeOneFrame() call.
+  bool frame_decoded_;
+
+  // Process H264 stream structures.
+  bool ProcessSPS(int sps_id);
+  bool ProcessPPS(int pps_id);
+  bool ProcessSlice(H264SliceHeader* slice_hdr);
+
+  // Initialize the current picture according to data in |slice_hdr|.
+  bool InitCurrPicture(H264SliceHeader* slice_hdr);
+
+  // Calculate picture order counts for the new picture
+  // on initialization of a new frame (see spec).
+  bool CalculatePicOrderCounts(H264SliceHeader* slice_hdr);
+
+  // Update PicNum values in pictures stored in DPB on creation of new
+  // frame (see spec).
+  void UpdatePicNums();
+
+  // Construct initial reference picture lists for use in decoding of
+  // P and B pictures (see 8.2.4 in spec).
+  void ConstructReferencePicListsP(H264SliceHeader* slice_hdr);
+  void ConstructReferencePicListsB(H264SliceHeader* slice_hdr);
+
+  // Helper functions for reference list construction, per spec.
+  int PicNumF(H264Picture *pic);
+  int LongTermPicNumF(H264Picture *pic);
+
+  // Perform the reference picture lists' modification (reordering), as
+  // specified in spec (8.2.4).
+  //
+  // |list| indicates list number and should be either 0 or 1.
+  bool ModifyReferencePicList(H264SliceHeader *slice_hdr, int list);
+
+  // Perform reference picture memory management operations (marking/unmarking
+  // of reference pictures, long term picture management, discarding, etc.).
+  // See 8.2.5 in spec.
+  bool HandleMemoryManagementOps();
+  void ReferencePictureMarking();
+
+  // Start processing a new frame.
+  bool StartNewFrame(H264SliceHeader* slice_hdr);
+
+  // All data for a frame received, process it and decode.
+  bool FinishPrevFrameIfPresent();
+
+  // Called after decoding, performs all operations to be done after decoding,
+  // including DPB management, reference picture marking and memory management
+  // operations.
+  // This will also output a picture if one is ready for output.
+  bool FinishPicture();
+
+  // Convert VideoCodecProfile to VAProfile and set it as active.
+  bool SetProfile(media::VideoCodecProfile profile);
+
+  // Parser in use.
+  H264Parser parser_;
+
+  // DPB in use.
+  H264DPB dpb_;
+
+  // Picture currently being processed/decoded.
+  scoped_ptr<H264Picture> curr_pic_;

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Why not a simple member?

[Pawel Osciak, 2012/03/21 18:40:35]

You mean a pointer?

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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

+
+  // Reference picture lists, constructed for each picture before decoding.
+  // Those lists are not owners of the pointers (DPB is).
+  PicsVector RefPicList0_;
+  PicsVector RefPicList1_;
+
+  // Global state values, needed in decoding. See spec.
+  int MaxPicOrderCntLsb_;
+  int MaxFrameNum_;
+  int MaxPicNum_;
+  int MaxLongTermFrameIdx_;
+
+  int frame_num_;
+  int prev_frame_num_;
+
+  // Values related to previously decoded reference picture.

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Does it not make sense to have a 
H264Picture last_ref_picture_;
?

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

A copy wouldn't involve mgmt pain and the wasted memory is in the 10s of bytes;
certainly irrelevant in this space.
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)

[Pawel Osciak, 2012/04/05 10:37:20]

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

[Ami GONE FROM CHROMIUM, 2012/04/09 02:41:47]

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.

+  bool prev_ref_has_memmgmnt5_;
+  int prev_ref_TopFieldOrderCnt_;
+  int prev_ref_PicOrderCntMsb_;
+  int prev_ref_pic_order_cnt_lsb_;
+  H264Picture::Field prev_ref_field_;
+
+  // Currently active SPS and PPS.
+  int curr_sps_id_;
+  int curr_pps_id_;
+
+  // Output picture size.
+  int pic_width_;
+  int pic_height_;
+

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

extra \n

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+
+  // Vaapi-related functions.

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

style guide requires functions before members.

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+
+  // Allocates VASurfaces and creates a VAContext for them.
+  bool CreateVASurfaces();
+
+  // Destroys allocated VASurfaces and related VAContext.
+  void DestroyVASurfaces();
+
+  // These queue up data for HW decoder to be commited on running HW decode.

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

committed

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  bool SendPPS();
+  bool SendIQMatrix();
+  bool SendVASliceParam(H264SliceHeader* slice_hdr);
+  bool SendSliceData(void* ptr, size_t size);
+  bool QueueSlice(H264SliceHeader* slice_hdr);
+
+  // Helper methods for filling HW structures.
+  void FillVAPicture(VAPictureH264 *va_pic, H264Picture* pic);
+  int FillVARefFramesFromDPB(VAPictureH264 *va_pics);
+
+  // Commits all pending data for HW decoder and starts HW decoder.
+  bool DecodePicture();
+
+  // Notifies client that a picture is ready for output.
+  bool OutputPic(H264Picture* pic);
+
+  // Data queued up for HW decoder, to be commited on next HW decode.
+  std::queue<VABufferID> pending_slice_bufs_;
+  std::queue<VABufferID> pending_va_bufs_;
+
+  // Manages binding of a client-provided output buffer (texture)
+  // to VASurface.
+  struct DecodeSurface {

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

fwd-declare here and define in .cc file?

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

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.

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+   public:
+    VASurfaceID va_surface_id;
+
+    // Client-provided ids.
+    int32 input_id;
+    int32 picture_buffer_id;
+    uint32 texture_id;
+
+    // Available for decoding (data no longer used for reference or output).
+    bool available;
+
+    // PicOrderCount
+    int poc;
+
+    // Pixmaps bound to this texture.
+    Pixmap x_pixmap;
+    GLXPixmap glx_pixmap;
+
+    DecodeSurface();

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

move these to the top of the class

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+    ~DecodeSurface();
+
+    // Bind the surface to a texture of the given |width| and |height|,
+    // allocating pixmaps as needed.
+    bool BindToTexture(int width, int height);

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

inline into ctor and replace bool ret value with setting available to false,
which callers can check post-construction?

[Pawel Osciak, 2012/03/21 18:40:35]

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.

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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.

[Pawel Osciak, 2012/04/05 10:37:20]

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.

+
+    // Unbind surface from texture and deallocate resources.
+    void UnbindFromTexture();

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

inline into dtor

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+
+    // Called before output to sync texture contents with the results
+    // of decode.
+    bool Sync();

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

not defined?

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+
+    // Initialize/destroy static state of this class (X Display and current

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

Why static?  Instead leave ownership w/ decoder object and pass refs in ctor. 
Then decoder is in charge of init/destroy.

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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

[Pawel Osciak, 2012/04/05 10:37:20]

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.

+    // FBConfig).
+    static bool Initialize(Display* x_display);
+    static void Destroy();
+
+   private:
+    static Display* x_display_;
+    // Used for texture binding.
+    static base::LazyInstance<GLXFBConfig> fb_config_;

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

I think LazyInstance can be dropped.

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  };
+
+  // Maps output_buffer_id to a decode surface. Used to look up surfaces
+  // on requests from the client.
+  typedef std::map<int32, linked_ptr<DecodeSurface> > DecodeSurfaces;
+  DecodeSurfaces decode_surfaces_;
+
+  // Number of decode surface currently available for decoding.
+  int num_available_decode_surfaces_;
+
+  // Maps decode surfaces to PicOrderCount, used to look up output buffers
+  // when a decision to output a picture has been made.
+  typedef std::map<int, DecodeSurface*> POCToDecodeSurfaces;

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

why not a linked_ptr?

[Pawel Osciak, 2012/03/21 18:40:35]

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

[Ami GONE FROM CHROMIUM, 2012/03/22 17:01:36]

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)

[Pawel Osciak, 2012/04/05 10:37:20]

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

[Ami GONE FROM CHROMIUM, 2012/04/09 02:41:47]

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.

+  POCToDecodeSurfaces poc_to_decode_surfaces_;
+
+  // The id of current input buffer, which will be associated with an
+  // output picture if a frame is decoded successfully.
+  int32 curr_input_id_;
+
+  // X/GLX handles.
+  Display* x_display_;
+  GLXContext parent_glx_context_;
+
+  // VA handles.
+  VADisplay va_display_;
+  VAConfigID va_config_id_;
+  VAContextID va_context_id_;
+  VAProfile profile_;
+
+  // Allocated VASurfaces.
+  VASurfaceID va_surface_ids_[kNumReqPictures];
+  // The number of the above surfaces successfully allocated and assigned
+  // to output buffers.
+  int num_assigned_vaapi_surfaces_;
+
+  // Assing a surface to PicOrderCount, removing it from the available surfaces

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/Assing/Assign/

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  // pool. Used when starting processing a new picture to get a free surface
+  // to decode too.

[Ami GONE FROM CHROMIUM, 2012/03/21 13:16:24]

s/too/to/

[Pawel Osciak, 2012/04/05 10:37:20]

Done.

+  bool AssignSurfaceToPoC(int poc);
+
+  // Unassign a surface when the data it contains is not needed for reference
+  // or display anymore, thus returning it to the available surfaces pool.
+  void UnassignSurfaceFromPoC(int poc);
+
+  // For posting the OutputPicCallback, provided by the client.
+  MessageLoop* msg_loop_;
+  void* output_pic_callback_arg_;
+  OutputPicCallbackPtr output_pic_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(VaapiH264Decoder);
+};
+
+#endif // CONTENT_COMMON_GPU_MEDIA_VAAPI_H264_DECODER_H_
+