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

content/common/gpu/media/h264_dpb.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 H.264 Decoded Picture Buffer
+// used in H264 decoders.
+
+#ifndef CONTENT_COMMON_GPU_MEDIA_H264_DPB_H_
+#define CONTENT_COMMON_GPU_MEDIA_H264_DPB_H_
+
+#include "h264_parser.h"
+
+#include <vector>
+
+#include "base/basictypes.h"
+
+// A picture (a frame or a field) in the H.264 spec sense.
+// See spec at http://www.itu.int/rec/T-REC-H.264
+struct H264Picture {
+  enum Field {
+    FIELD_NONE,
+    FIELD_TOP,
+    FIELD_BOTTOM,
+  };
+
+  // Values calculated per H.264 specification or taken from slice header.
+  // See spec for more details on each.
+  int TopFieldOrderCnt;

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

Naming in this file is inconsistent.  Please style-guide it up.

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

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.

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

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

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

Done.

+  int BottomFieldOrderCnt;
+  int PicOrderCnt;
+  int PicOrderCntMsb;
+  int pic_order_cnt_lsb;
+
+  int PicNum;
+  int LongTermPicNum;
+  int frame_num; // from slice header
+  int FrameNumWrap;
+  int LongTermFrameIdx;
+
+  bool idr; // IDR picture?
+  bool ref; // reference picture?
+  bool long_term; // long term reference picture?
+  bool outputted;
+  // Does memory management op 5 needs to be executed after this
+  // picture has finished decoding?
+  bool mem_mgmt_5;
+
+  Field field;
+
+  // Values from slice_hdr to be used during reference marking and
+  // memory management after finishing this picture.
+  bool long_term_reference_flag;
+  bool adaptive_ref_pic_marking_mode_flag;
+  H264DecRefPicMarking ref_pic_marking[H264SliceHeader::kRefListSize];
+
+  // Utility function objects

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

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?

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

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.

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

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)

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

Done. And got rid of majority of them.

+  struct POCAscCompare {
+    bool operator()(const H264Picture* a, const H264Picture* b) const {
+      return a->PicOrderCnt < b->PicOrderCnt;
+    }
+  };
+
+  struct POCDescCompare {
+    bool operator()(const H264Picture* a, const H264Picture* b) const {
+      return a->PicOrderCnt > b->PicOrderCnt;
+    }
+  };
+
+  struct PicNumDescCompare {
+    bool operator()(const H264Picture* a, const H264Picture* b) const {
+      return a->PicNum > b->PicNum;
+    }
+  };
+
+  struct LongTermPicNumAscCompare {
+    bool operator()(const H264Picture* a, const H264Picture* b) const {
+      return a->LongTermPicNum < b->LongTermPicNum;
+    }
+  };
+
+  struct FrameNumWrapAscCompare {
+    bool operator()(H264Picture*& a, H264Picture*& b) const {
+      return a->FrameNumWrap < b->FrameNumWrap;
+    }
+  };
+
+  struct IsLongTermRefPic
+      : public std::unary_function<H264Picture*, bool> {
+    bool operator()(const H264Picture* pic) const {
+      return (pic->ref && pic->long_term);
+    }
+  };
+
+  struct IsUnused {
+    bool operator()(H264Picture*& pic) {
+      return pic->outputted && !pic->ref;
+    }
+  };
+
+  struct IsOutputted {
+    bool operator()(H264Picture*& pic) {
+      return pic->outputted;
+    }
+  };
+
+  struct IsReference {
+    bool operator()(H264Picture*& pic) {
+      return pic->ref;
+    }
+  };
+};
+
+typedef std::vector<H264Picture*> PicsVector;

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

Move into H264Picture and and rename to just Vector?

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

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

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

Umm, should it instead by a typedef inside H264DPB?

+
+// DPB - Decoded Picture Buffer.
+// Stores decoded pictures that will be used for future display
+// and/or reference.
+class H264DPB {

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

Sad that the spec defines DPB as a buffer of decoded pictures, but other VDA
terminology uses PictureBuffer for a single image :(

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

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.

+ public:
+  H264DPB();
+  ~H264DPB();
+
+  // Remove unused (not reference and already outputted) pictures from DPB.
+  void RemoveUnused();
+
+  // Remove a picture by its PicOrderCnt.
+  void RemoveByPOC(int poc);
+
+  // Clear DPB.
+  void Clear();
+
+  // Store picture in DPB. DPB takes ownership of its resources.
+  void StorePic(H264Picture* pic);

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

If you passed by scoped_ptr you wouldn't have to document ownership transfer.

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

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.

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

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

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

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.

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

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.

+
+  // Return the number of reference pictures in DPB.
+  int CountRefPics();
+
+  // Mark all pictures in DPB as unused for reference.
+  void MarkAllUnusedForRef();
+
+  // Return a short-term reference picture by its PicNum.
+  H264Picture* GetShortRefPicByPicNum(int pic_num);

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

Assuming this isn't relinquishing ownership, IWBN to return const-ref instead of
pointer here (and below).

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

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

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

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.

+
+  // Return a long-term reference picture by its LongTermPicNum.
+  H264Picture* GetLongRefPicByLongTermPicNum(int pic_num);
+
+  // Return the reference picture with lowest FrameNum. Used for sliding
+  // window memory management.
+  H264Picture* GetLowestFrameNumWrapRefPic();
+
+  // Append all pictures that have not been outputted yet to the passed |out|
+  // vector, sorted by lowest PicOrderCnt (in output order).
+  void GetNotOutputtedPicsSortedByPOCAppending(PicsVector& out);
+
+  // Append all short term reference pictures to the passed |out| vector.
+  void GetShortTermRefPicsAppending(PicsVector& out);
+
+  // Append all long term reference pictures to the passed |out| vector.
+  void GetLongTermRefPicsAppending(PicsVector& out);
+
+  // Iterators for direct access to DPB contents.

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

These will be invalidated by mutating methods elsewhere in this class.  Do you
want to document anything about that?

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

Done.

+  PicsVector::iterator begin() { return pics_.begin(); }
+  PicsVector::iterator end() { return pics_.end(); }
+  PicsVector::reverse_iterator rbegin() { return pics_.rbegin(); }
+  PicsVector::reverse_iterator rend() { return pics_.rend(); }
+
+  size_t size() const { return pics_.size(); }
+  bool IsFull() const { return pics_.size() == kDPBMaxSize; }
+
+  // Per H264 spec, increase to 32 if interlaced video is supported.
+  enum { kDPBMaxSize = 16 };
+
+ private:
+

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

extra newline

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

Done.

+  // Get a reference picture by its (LongTerm)PicNum, short term if |long_term|
+  // is false, long term otherwise.
+  H264Picture* GetRefPicByPicNum(int pic_num, bool long_term);
+
+  // Remove a picture from DPB, freeing its resources.
+  void RemovePic(const PicsVector::iterator iter);
+
+  // TODO posciak: consider using std::list instead.
+  PicsVector pics_;
+
+  DISALLOW_COPY_AND_ASSIGN(H264DPB);
+};
+
+#endif // CONTENT_COMMON_GPU_MEDIA_H264_DPB_H_
+