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

content/common/gpu/media/vaapi_h264_decoder.cc

Index: content/common/gpu/media/vaapi_h264_decoder.cc
diff --git a/content/common/gpu/media/vaapi_h264_decoder.cc b/content/common/gpu/media/vaapi_h264_decoder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8f2a274cd20a0f7012b79c3602ac6fd730d6f12a
--- /dev/null
+++ b/content/common/gpu/media/vaapi_h264_decoder.cc
@@ -0,0 +1,2080 @@
+// 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.
+
+#include "vaapi_h264_decoder.h"
+
+#include <algorithm>
+#include <dlfcn.h>
+
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "third_party/libva/va/va.h"
+#include "third_party/libva/va/va_x11.h"
+#include "ui/gfx/gl/gl_bindings.h"
+
+#define VA_SUCCESS_OR_ERROR(vares, err_msg)                   \

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

this name is confusing
(why, yes, I'm sure we either had success or had an error ;))

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

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

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

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.

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

Done.

+  do {                                                        \
+    if ((vares) != VA_STATUS_SUCCESS) {                       \
+      DLOG(ERROR) << (err_msg);                               \

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

s/DLOG/DVLOG/ everywhere unless you have a reason not to.

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

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

+      DLOG(ERROR) << "VA error: " << vaapi_vaErrorStr(vares); \

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

Can these be a single LOG?

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

Done.

+    }                                                         \
+  } while(0)
+
+#define VA_SUCCESS_OR_RETURN(vares, err_msg, ret)             \
+  do {                                                        \
+    if ((vares) != VA_STATUS_SUCCESS) {                       \
+      DLOG(ERROR) << (err_msg);                               \
+      DLOG(ERROR) << "VA error: " << vaapi_vaErrorStr(vares); \
+      return (ret);                                           \

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

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?

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

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

+    }                                                         \
+  } while (0)
+
+void *vaapi_handle = dlopen("libva.so", RTLD_NOW);
+void *vaapi_x11_handle = dlopen("libva-x11.so", RTLD_NOW);
+void *vaapi_glx_handle = dlopen("libva-glx.so", RTLD_NOW);
+
+typedef VADisplay (*vaapiGetDisplayGLX)(Display *dpy);

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

Types need to be Capitalized (here and elsewhere)

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

Done.

+

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

remove the newlines between these typedefs?

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

Done.

+typedef int (*vaapiDisplayIsValid)(VADisplay dpy);
+
+typedef VAStatus (*vaapiInitialize)(VADisplay dpy,
+                                    int *major_version,
+                                    int *minor_version);
+
+typedef VAStatus (*vaapiTerminate)(VADisplay dpy);
+
+typedef VAStatus (*vaapiGetConfigAttributes)(VADisplay dpy,
+                                             VAProfile profile,
+                                             VAEntrypoint entrypoint,
+                                             VAConfigAttrib *attrib_list,
+                                             int num_attribs);
+
+typedef VAStatus (*vaapiCreateConfig)(VADisplay dpy,
+                                      VAProfile profile,
+                                      VAEntrypoint entrypoint,
+                                      VAConfigAttrib *attrib_list,
+                                      int num_attribs,
+                                      VAConfigID *config_id);
+
+typedef VAStatus (*vaapiDestroyConfig)(VADisplay dpy, VAConfigID config_id);
+
+typedef VAStatus (*vaapiCreateSurfaces)(VADisplay dpy,
+                                        int width,
+                                        int height,
+                                        int format,
+                                        int num_surfaces,
+                                        VASurfaceID *surfaces);
+
+typedef VAStatus (*vaapiDestroySurfaces)(VADisplay dpy,
+                                         VASurfaceID *surfaces,
+                                         int num_surfaces);
+
+typedef VAStatus (*vaapiCreateContext)(VADisplay dpy,
+                                       VAConfigID config_id,
+                                       int picture_width,
+                                       int picture_height,
+                                       int flag,
+                                       VASurfaceID *render_targets,
+                                       int num_render_targets,
+                                       VAContextID *context);
+
+typedef VAStatus (*vaapiDestroyContext)(VADisplay dpy, VAContextID context);
+
+typedef VAStatus (*vaapiPutSurface)(VADisplay dpy,
+                                    VASurfaceID surface,
+                                    Drawable draw,
+                                    short srcx,
+                                    short srcy,
+                                    unsigned short srcw,
+                                    unsigned short srch,
+                                    short destx,
+                                    short desty,
+                                    unsigned short destw,
+                                    unsigned short desth,
+                                    VARectangle *cliprects,
+                                    unsigned int number_cliprects,
+                                    unsigned int flags);
+
+typedef VAStatus (*vaapiSyncSurface)(VADisplay dpy, VASurfaceID render_target);
+
+typedef VAStatus (*vaapiBeginPicture)(VADisplay dpy,
+                                      VAContextID context,
+                                      VASurfaceID render_target);
+
+typedef VAStatus (*vaapiRenderPicture)(VADisplay dpy,
+                                       VAContextID context,
+                                       VABufferID *buffers,
+                                       int num_buffers);
+
+typedef VAStatus (*vaapiEndPicture)(VADisplay dpy, VAContextID context);
+
+typedef VAStatus (*vaapiCreateBuffer)(VADisplay dpy,
+                                      VAContextID context,
+                                      VABufferType type,
+                                      unsigned int size,
+                                      unsigned int num_elements,
+                                      void *data,
+                                      VABufferID *buf_id);
+
+typedef const char* (*vaapiErrorStr)(VAStatus error_status);
+
+vaapiGetDisplayGLX vaapi_vaGetDisplayGLX =

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

s/vaapi_vaGetDisplayGLX/VAAPI_GetDisplayGLX/
(and similar renaming throughout)

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

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.

+    reinterpret_cast<vaapiGetDisplayGLX>(dlsym(vaapi_glx_handle,

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

does using a macro for this make sense?

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

Done.

+                                               "vaGetDisplayGLX"));
+
+vaapiDisplayIsValid vaapi_vaDisplayIsValid =
+    reinterpret_cast<vaapiDisplayIsValid>(dlsym(vaapi_handle,
+                                                "vaDisplayIsValid"));
+
+vaapiInitialize vaapi_vaInitialize =
+    reinterpret_cast<vaapiInitialize>(dlsym(vaapi_handle, "vaInitialize"));
+
+vaapiTerminate vaapi_vaTerminate =
+    reinterpret_cast<vaapiTerminate>(dlsym(vaapi_handle, "vaTerminate"));
+
+vaapiGetConfigAttributes vaapi_vaGetConfigAttributes =
+    reinterpret_cast<vaapiGetConfigAttributes>(dlsym(vaapi_handle,
+                                                     "vaGetConfigAttributes"));
+
+vaapiCreateConfig vaapi_vaCreateConfig =
+    reinterpret_cast<vaapiCreateConfig>(dlsym(vaapi_handle, "vaCreateConfig"));
+
+vaapiDestroyConfig vaapi_vaDestroyConfig =
+    reinterpret_cast<vaapiDestroyConfig>(dlsym(vaapi_handle,
+                                               "vaDestroyConfig"));
+
+vaapiCreateSurfaces vaapi_vaCreateSurfaces =
+    reinterpret_cast<vaapiCreateSurfaces>(dlsym(vaapi_handle,
+                                                "vaCreateSurfaces"));
+
+vaapiDestroySurfaces vaapi_vaDestroySurfaces =
+    reinterpret_cast<vaapiDestroySurfaces>(dlsym(vaapi_handle,
+                                                 "vaDestroySurfaces"));
+
+vaapiCreateContext vaapi_vaCreateContext =
+    reinterpret_cast<vaapiCreateContext>(dlsym(vaapi_handle,
+                                               "vaCreateContext"));
+
+vaapiDestroyContext vaapi_vaDestroyContext =
+    reinterpret_cast<vaapiDestroyContext>(dlsym(vaapi_handle,
+                                                "vaDestroyContext"));
+
+vaapiPutSurface vaapi_vaPutSurface =
+    reinterpret_cast<vaapiPutSurface>(dlsym(vaapi_x11_handle, "vaPutSurface"));
+
+vaapiSyncSurface vaapi_vaSyncSurface =
+    reinterpret_cast<vaapiSyncSurface>(dlsym(vaapi_x11_handle,
+                                             "vaSyncSurface"));
+
+vaapiBeginPicture vaapi_vaBeginPicture =
+    reinterpret_cast<vaapiBeginPicture>(dlsym(vaapi_handle, "vaBeginPicture"));
+
+vaapiRenderPicture vaapi_vaRenderPicture =
+    reinterpret_cast<vaapiRenderPicture>(dlsym(vaapi_handle,
+                                               "vaRenderPicture"));
+
+vaapiEndPicture vaapi_vaEndPicture =
+    reinterpret_cast<vaapiEndPicture>(dlsym(vaapi_handle, "vaEndPicture"));
+
+vaapiCreateBuffer vaapi_vaCreateBuffer =
+    reinterpret_cast<vaapiCreateBuffer>(dlsym(vaapi_handle, "vaCreateBuffer"));
+
+vaapiErrorStr vaapi_vaErrorStr =
+    reinterpret_cast<vaapiErrorStr>(dlsym(vaapi_handle, "vaErrorStr"));
+
+static bool AreVaapiFunctionPointersInitialized() {
+  return (vaapi_vaGetDisplayGLX

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

drop the parens?

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

Done.

+       && vaapi_vaDisplayIsValid
+       && vaapi_vaInitialize
+       && vaapi_vaTerminate
+       && vaapi_vaGetConfigAttributes
+       && vaapi_vaCreateConfig
+       && vaapi_vaDestroyConfig
+       && vaapi_vaCreateSurfaces
+       && vaapi_vaDestroySurfaces
+       && vaapi_vaCreateContext
+       && vaapi_vaDestroyContext
+       && vaapi_vaPutSurface
+       && vaapi_vaSyncSurface
+       && vaapi_vaBeginPicture
+       && vaapi_vaRenderPicture
+       && vaapi_vaEndPicture
+       && vaapi_vaCreateBuffer
+       && vaapi_vaErrorStr);
+}
+
+VaapiH264Decoder::VaapiH264Decoder() {
+  // Has to be before Reset() so it does not mark any surfaces as available.
+  num_assigned_vaapi_surfaces_ = 0;
+  Reset();
+  curr_sps_id_ = -1;
+  curr_pps_id_ = -1;
+  pic_width_ = -1;
+  pic_height_ = -1;
+  MaxFrameNum_ = 0;
+  MaxPicNum_ = 0;
+  MaxLongTermFrameIdx_ = 0;
+  MaxPicOrderCntLsb_ = 0;
+  state_ = kUninitialized;
+}
+
+VaapiH264Decoder::~VaapiH264Decoder() {
+  Destroy();
+}
+
+// This puts the decoder in state where it keeps stream data and is ready
+// to resume playback from a random location in the stream, but drops all
+// inputs and outputs and makes all surfaces available for use.
+void VaapiH264Decoder::Reset() {
+  frame_decoded_ = false;
+
+  curr_pic_.reset();
+
+  frame_num_ = 0;
+  prev_frame_num_ = -1;
+
+  prev_ref_has_memmgmnt5_ = false;
+  prev_ref_TopFieldOrderCnt_ = -1;
+  prev_ref_PicOrderCntMsb_ = -1;
+  prev_ref_pic_order_cnt_lsb_ = -1;
+  prev_ref_field_ = H264Picture::FIELD_NONE;
+
+  pending_slice_bufs_ = std::queue<VABufferID>();
+  pending_va_bufs_ = std::queue<VABufferID>();
+
+  RefPicList0_.clear();
+  RefPicList1_.clear();
+
+  poc_to_decode_surfaces_.clear();
+
+  for (DecodeSurfaces::iterator iter = decode_surfaces_.begin();
+       iter != decode_surfaces_.end(); ++iter)
+    iter->second->available = true;
+  num_available_decode_surfaces_ = num_assigned_vaapi_surfaces_;

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

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

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

Yeah, good idea. Done.

+
+  dpb_.Clear();
+  parser_.Reset();
+
+  // Still initialized and ready to decode

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

this is a lie during the call from the ctor

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

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

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

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.

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

Done.

+  state_ = kAfterReset;
+}
+
+void VaapiH264Decoder::Destroy() {

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

early-return if state_==kUninitialized ?

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

Done.

+  VAStatus va_status;
+
+  Reset();
+
+  switch (state_) {

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

Reset() just set state_; why is this switch necessary?

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

In case we call it without Reset()?

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

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

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

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

+    case kDecoding:
+    case kAfterReset:
+    case kError:
+      DecodeSurface::Destroy();
+      DestroyVASurfaces();
+      // fallthrough
+    case kInitialized:
+      va_status = vaapi_vaDestroyConfig(va_display_, va_config_id_);
+      VA_SUCCESS_OR_ERROR(va_status, "vaDestroyConfig failed");
+      va_status = vaapi_vaTerminate(va_display_);
+      VA_SUCCESS_OR_ERROR(va_status, "vaTerminate failed");
+      // fallthrough
+    case kUninitialized:
+      break;
+
+    default:

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

better to drop default: and let the compiler yell at you if you forgot a case.

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

Done.

+      NOTREACHED();
+  }
+
+  state_ = kUninitialized;
+}
+
+bool VaapiH264Decoder::HasHWSupport() {
+  return AreVaapiFunctionPointersInitialized();

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

This doesn't check HW support, only presence of libraries.

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

Done.

+}
+
+// Maps Profile enum values to VaProfile values.
+bool VaapiH264Decoder::SetProfile(media::VideoCodecProfile profile) {
+  switch (profile) {
+    case media::H264PROFILE_BASELINE:
+      profile_ = VAProfileH264Baseline;
+      break;
+    case media::H264PROFILE_MAIN:
+      profile_ = VAProfileH264Main;
+      break;
+    case media::H264PROFILE_HIGH:
+      profile_ = VAProfileH264High;
+      break;
+    default:
+      DLOG(INFO) << "Unsupported profile: " << profile;
+      return false;
+  }
+

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

extra \n

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

Done.

+  return true;
+}
+
+// Has to be run in GLXContext thread

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

replace this comment (and others like it) with DCHECK at the top of the
function.

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

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.

+bool VaapiH264Decoder::Initialize(media::VideoCodecProfile profile,
+                                  Display* x_display,
+                                  GLXContext glx_context,
+                                  MessageLoop* msg_loop,
+                                  OutputPicCallbackPtr output_pic_callback,
+                                  void *arg) {
+  VAStatus va_res;

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

first use is at l.349; move this there.

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

Done.

+  DCHECK_EQ(state_, kUninitialized);
+
+  msg_loop_ = msg_loop;
+  output_pic_callback_ = output_pic_callback;
+  output_pic_callback_arg_ = arg;
+
+  x_display_ = x_display;
+  parent_glx_context_ = glx_context;
+
+  if (!SetProfile(profile)) {
+    DLOG(INFO) << "Unsupported profile";
+    return false;
+  }
+
+  if (!HasHWSupport()) {
+    DLOG(ERROR) << "Hardware not supported or could not load libva";
+    return false;
+  }
+
+  va_display_ = vaapi_vaGetDisplayGLX(x_display_);
+  if (!vaapi_vaDisplayIsValid(va_display_)) {
+    DLOG(ERROR) << "Could not get a valid VA display";
+    return false;
+  }
+
+  int major_version, minor_version;
+  va_res = vaapi_vaInitialize(va_display_, &major_version, &minor_version);
+  VA_SUCCESS_OR_RETURN(va_res, "vaInitialize failed", false);
+  DVLOG(1) << "VAAPI version: " << major_version << "." << minor_version;
+
+  VAConfigAttrib attrib;
+  attrib.type = VAConfigAttribRTFormat;
+
+  VAEntrypoint entrypoint = VAEntrypointVLD;
+  va_res = vaapi_vaGetConfigAttributes(va_display_, profile_, entrypoint,
+                                       &attrib, 1);
+  VA_SUCCESS_OR_RETURN(va_res, "vaGetConfigAttributes failed", false);
+
+  if (!(attrib.value & VA_RT_FORMAT_YUV420)) {
+    DLOG(INFO) << "YUV420 not supported";

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

s/DLOG/DVLOG/
s/INFO/ERROR/

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

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?

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

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.

+    return false;
+  }
+
+  va_res = vaapi_vaCreateConfig(va_display_, profile_, entrypoint,
+                                &attrib, 1, &va_config_id_);
+  VA_SUCCESS_OR_RETURN(va_res, "vaCreateConfig failed", false);
+
+  if (!DecodeSurface::Initialize(x_display_)) {
+    DLOG(ERROR) << "Could not get a usable FBConfig";
+    return false;
+  }
+
+  state_ = kInitialized;
+
+  return true;
+}
+
+VaapiH264Decoder::DecodeSurface::DecodeSurface()
+  : available(false)
+{}

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

opening brace belongs on previous line

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

Done.

+
+VaapiH264Decoder::DecodeSurface::~DecodeSurface() {
+  UnbindFromTexture();
+}
+
+// static
+bool VaapiH264Decoder::DecodeSurface::Initialize(Display* x_display) {
+  const int fbconfig_attr[] = {
+    GLX_DRAWABLE_TYPE, GLX_PIXMAP_BIT,
+    GLX_BIND_TO_TEXTURE_TARGETS_EXT, GLX_TEXTURE_2D_BIT_EXT,
+    GLX_BIND_TO_TEXTURE_RGB_EXT, GL_TRUE,
+    GLX_Y_INVERTED_EXT, GL_TRUE,

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

what's this about?

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

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

+    GL_NONE,
+  };
+
+  DCHECK(x_display);
+  x_display_ = x_display;
+
+  int num_fbconfigs;
+  GLXFBConfig* glx_fb_configs = glXChooseFBConfig(x_display_,
+      DefaultScreen(x_display_), fbconfig_attr, &num_fbconfigs);
+  if (!glx_fb_configs)
+    return false;
+
+  fb_config_.Get() = glx_fb_configs[0];
+
+  return true;
+}
+
+// static
+void VaapiH264Decoder::DecodeSurface::Destroy() {
+  if (fb_config_.Get())
+    free(fb_config_.Get());
+}
+
+
+static int x11_error = 0;

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

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

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

Well, I was restoring them after use ;)

+static int (*old_error_handler)(Display *, XErrorEvent *);
+
+static int X11ErrorHandler(Display *dpy, XErrorEvent *e) {
+  char buf[512];
+  LOG(ERROR) << "X11 error: " << (unsigned int)(e->error_code)
+             << " on display: " << dpy;
+  XGetErrorText(dpy, e->error_code, buf, 512);
+  LOG(ERROR) << "Error text: " << buf;
+  return 0;
+}
+
+static void TrapX11Errors() {
+  x11_error = 0;
+  old_error_handler = XSetErrorHandler(X11ErrorHandler);
+}
+
+static void UntrapX11Errors() {
+  XSetErrorHandler(old_error_handler);
+}
+
+// Must be run on the GLX thread.
+bool VaapiH264Decoder::DecodeSurface::BindToTexture(int width, int height) {
+  TrapX11Errors();
+
+  glEnable(GL_TEXTURE_2D);
+  glBindTexture(GL_TEXTURE_2D, texture_id);
+  glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+  glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+
+  XWindowAttributes win_attr;
+  int screen = DefaultScreen(x_display_);
+  XGetWindowAttributes(x_display_, RootWindow(x_display_, screen), &win_attr);
+  x_pixmap = XCreatePixmap(x_display_, RootWindow(x_display_, screen),
+                           width, height, win_attr.depth);
+  if (!x_pixmap) {
+    DLOG(ERROR) << "Failed creating an X Pixmap for TFP";
+    return false;
+  }
+
+  DCHECK(fb_config_.Get());
+
+  static const int pixmap_attr[] = {
+    GLX_TEXTURE_TARGET_EXT, GLX_TEXTURE_2D_EXT,
+    GLX_TEXTURE_FORMAT_EXT, GLX_TEXTURE_FORMAT_RGB_EXT,
+    GL_NONE,
+  };
+
+  glx_pixmap = glXCreatePixmap(x_display_, fb_config_.Get(), x_pixmap,
+                               pixmap_attr);
+
+  glBindTexture(GL_TEXTURE_2D, texture_id);
+  glXBindTexImageEXT(x_display_, glx_pixmap, GLX_FRONT_LEFT_EXT, NULL);
+  // TODO posciak: verify whether needed
+  XSync(x_display_, False);

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

I don't think it's ok to have XSync's without documented reason (I expect them
not to be needed).

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

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

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

Done.

+
+  UntrapX11Errors();
+  return true;
+}
+
+// Must be run on the GLX thread.
+void VaapiH264Decoder::DecodeSurface::UnbindFromTexture() {
+  TrapX11Errors();
+
+  glXReleaseTexImageEXT(x_display_, glx_pixmap, GLX_FRONT_LEFT_EXT);
+  // TODO posciak: verify whether needed
+  XSync(x_display_, False);
+  glXDestroyGLXPixmap(x_display_, glx_pixmap);
+  XFreePixmap(x_display_, x_pixmap);
+
+  UntrapX11Errors();
+}
+
+// Must be run on decoder thread
+void VaapiH264Decoder::ReusePictureBuffer(int32 picture_buffer_id) {
+  DecodeSurfaces::iterator it = decode_surfaces_.find(picture_buffer_id);
+  if (it == decode_surfaces_.end()) {
+    DLOG(ERROR) << "Asked to reuse an invalid buffer";
+    return;
+  }
+
+  UnassignSurfaceFromPoC(it->second->poc);
+}
+
+// Must be run in GLXContext thread.
+bool VaapiH264Decoder::AssignPictureBuffer(int32 picture_buffer_id,
+                                           uint32 texture_id) {
+  scoped_ptr<DecodeSurface> dec_surface(new DecodeSurface());

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

s/scoped/linked/

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

Done.

+
+  DCHECK_EQ(state_, kDecoding);

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

how can you be sure?  What if an error occurred but was only delivered after the
AssignPictureBuffer task was queued?

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

There can be no threading here yet.

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

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.

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

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.
 
VAVDA is not really ready for this yet. I don't think other VDAs are though
either...

+
+  if (num_assigned_vaapi_surfaces_ >= GetRequiredNumOfPictures()) {
+    DLOG(ERROR) << "Got more surfaces than required";
+    return false;
+  }
+
+  // This will not work if we start using VDA.DismissPicture()
+  dec_surface->va_surface_id = va_surface_ids_[num_assigned_vaapi_surfaces_++];
+  dec_surface->picture_buffer_id = picture_buffer_id;
+  dec_surface->texture_id = texture_id;
+
+  DVLOG(2) << "New picture assigned, texid: " << dec_surface->texture_id
+           << "va display: " << va_display_;
+
+  if (!dec_surface->BindToTexture(pic_width_, pic_height_)) {
+    DLOG(ERROR) << "Error binding VASurface to a texture";
+    return false;
+  }
+
+  dec_surface->available = true;
+  ++num_available_decode_surfaces_;
+
+  DVLOG(2) << "Pic buf id: " << dec_surface->picture_buffer_id
+           << " will use va surface " << dec_surface->va_surface_id
+           << ", texture id: " << dec_surface->texture_id;
+
+  bool inserted = decode_surfaces_.insert(std::make_pair(picture_buffer_id,
+                               make_linked_ptr(dec_surface.release()))).second;

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

indent is wrong (and make_linked_ptr/release shouldn't be necessary after you
change dec_surface to be a linked_ptr).

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

Done.

+  DCHECK(inserted);
+
+  return true;
+}
+
+bool VaapiH264Decoder::CreateVASurfaces() {
+  DCHECK_NE(pic_width_, -1);
+  DCHECK_NE(pic_height_, -1);
+  DCHECK_EQ(state_, kInitialized);
+
+  // Allocate VASurfaces in driver.
+  VAStatus va_res = vaapi_vaCreateSurfaces(va_display_, pic_width_,
+                                pic_height_, VA_RT_FORMAT_YUV420,

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

indentation here and elsewhere is off.  please make a pass.

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

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

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

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)

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

Done.

+                                GetRequiredNumOfPictures(), va_surface_ids_);
+  VA_SUCCESS_OR_RETURN(va_res, "vaCreateSurfaces failed", false);
+
+  // How many VA Surfaces are assigned/bound to output pictures.
+  num_assigned_vaapi_surfaces_ = 0;
+
+  // And create a context associated with them.
+  va_res = vaapi_vaCreateContext(va_display_, va_config_id_,
+                                 pic_width_, pic_height_, VA_PROGRESSIVE,
+                                 va_surface_ids_, GetRequiredNumOfPictures(),
+                                 &va_context_id_);
+  VA_SUCCESS_OR_RETURN(va_res, "vaCreateContext failed", false);
+
+  return true;
+}
+
+void VaapiH264Decoder::DestroyVASurfaces() {
+  VAStatus va_status;

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

please standardize on a single name for this throughout the file.
I think "res" is fine (as opposed to va_res/va_status/etc).

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

Done.

+  DCHECK(state_ == kDecoding || state_ == kError || state_ == kAfterReset);
+
+  decode_surfaces_.clear();
+
+  va_status = vaapi_vaDestroyContext(va_display_, va_context_id_);
+  VA_SUCCESS_OR_ERROR(va_status, "vaDestroyContext failed");
+
+  va_status = vaapi_vaDestroySurfaces(va_display_, va_surface_ids_,
+                                      GetRequiredNumOfPictures());
+  VA_SUCCESS_OR_ERROR(va_status, "vaDestroyContext failed");
+}
+
+void VaapiH264Decoder::FillVAPicture(VAPictureH264 *va_pic, H264Picture* pic) {
+    va_pic->picture_id =
+        poc_to_decode_surfaces_[pic->PicOrderCnt]->va_surface_id;

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

operator[] will add a NULL pointer if the index is not found.  Ok?

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

Done.

+    DCHECK(va_pic->picture_id);
+
+    va_pic->frame_idx = pic->frame_num;
+    va_pic->flags = 0;
+
+    switch (pic->field) {
+      case H264Picture::FIELD_NONE:
+        break;
+      case H264Picture::FIELD_TOP:
+        va_pic->flags |= VA_PICTURE_H264_TOP_FIELD;
+        break;
+      case H264Picture::FIELD_BOTTOM:
+        va_pic->flags |= VA_PICTURE_H264_BOTTOM_FIELD;
+        break;
+      default:
+        NOTREACHED();

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

drop default: clause.

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

Done.

+    }
+
+    if (pic->ref)
+        va_pic->flags |= pic->long_term ? VA_PICTURE_H264_LONG_TERM_REFERENCE
+                                        : VA_PICTURE_H264_SHORT_TERM_REFERENCE;
+
+    va_pic->TopFieldOrderCnt = pic->TopFieldOrderCnt;
+    va_pic->BottomFieldOrderCnt = pic->BottomFieldOrderCnt;
+}
+
+int VaapiH264Decoder::FillVARefFramesFromDPB(VAPictureH264 *va_pics) {
+    PicsVector::reverse_iterator rit;
+    int i;
+
+    // Return reference frames in reverse order of insertion

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

what's the reverse order about?

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

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.

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

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.

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

Done.

+    for (rit = dpb_.rbegin(), i = 0; rit != dpb_.rend() && i < 16; ++rit)

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

s/16/kSomething/

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

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

+        if ((*rit)->ref)

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

multi-line for bodies require braces.
here & elsewhere; please make a pass to make sure this is taken care of for all
control structures.

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

Done.

+            FillVAPicture(&va_pics[i++], *rit);
+
+    return i;
+}
+
+// Can only be called when all surfaces are already bound
+// to textures (cannot be run at the same time as AssignPictureBuffer).
+bool VaapiH264Decoder::AssignSurfaceToPoC(int poc) {
+  // Find a surface not currently holding data used for reference and/or
+  // to be displayed and mark it as used.
+  DecodeSurfaces::iterator iter = decode_surfaces_.begin();
+  for (; iter != decode_surfaces_.end(); ++iter) {
+    if (iter->second->available) {
+      iter->second->available = false;
+      --num_available_decode_surfaces_;
+      DCHECK_GE(num_available_decode_surfaces_, 0);
+      break;
+    }
+  }
+
+  if (iter == decode_surfaces_.end())
+    return false;
+
+  // Assign the current input_id to it, to be used when returning to client.
+  iter->second->input_id = curr_input_id_;
+  iter->second->poc = poc;
+  poc_to_decode_surfaces_[poc] = iter->second.get();

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

not worried about overwriting a previous value? 
(why use operator[] instead of .insert() + DCHECK ret val?

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

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

+  DVLOG(4) << "Will use surface "
+           << poc_to_decode_surfaces_[poc]->va_surface_id
+           << " for POC " << poc << " input ID: " << iter->second->input_id;
+
+  return true;
+}
+
+// Can only be called when all surfaces are already bound
+// to textures (cannot be run at the same time as AssignPictureBuffer).
+void VaapiH264Decoder::UnassignSurfaceFromPoC(int poc) {
+  POCToDecodeSurfaces::iterator it = poc_to_decode_surfaces_.find(poc);
+  /*DCHECK(it != poc_to_decode_surfaces_.end());*/

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

drop?

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

Done.

+
+  DVLOG(4) << "POC " << poc << "no longer using surface "
+           << it->second->va_surface_id;
+  it->second->available = true;
+  poc_to_decode_surfaces_.erase(it);
+
+  ++num_available_decode_surfaces_;
+}
+
+// Fill VAPictureH264 with default/neutral values.
+static void InitVAPicture(VAPictureH264* va_pic) {
+  memset(va_pic, 0, sizeof(*va_pic));
+  va_pic->picture_id = 0xffffffff;

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

kuint32max?
(but why set it to this?)

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

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

+  va_pic->flags = VA_PICTURE_H264_INVALID;
+}
+
+// Fill a VAPictureParameterBufferH264 to be later sent to the HW decoder.
+bool VaapiH264Decoder::SendPPS() {
+  VAPictureParameterBufferH264 pic_param;
+  VABufferID pic_param_buf_id;
+
+  H264PPS* pps = parser_.GetPPS(curr_pps_id_);
+  DCHECK(pps);
+
+  H264SPS* sps = parser_.GetSPS(pps->seq_parameter_set_id);
+  DCHECK(sps);
+
+  DCHECK(curr_pic_.get());
+
+  memset(&pic_param, 0, sizeof(VAPictureParameterBufferH264));
+
+  pic_param.picture_width_in_mbs_minus1 = sps->pic_width_in_mbs_minus1;

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

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

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

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?

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

Can the parser not populate as much as it can and then have this class populate
the decoder-discovered bits?
Understood, but see elsewhere.
This reason is the least-compelling to me.
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.

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

Done.

+  // This assumes non-interlaced video
+  pic_param.picture_height_in_mbs_minus1 = sps->pic_height_in_map_units_minus1;
+  pic_param.bit_depth_luma_minus8 = sps->bit_depth_luma_minus8;
+  pic_param.bit_depth_chroma_minus8 = sps->bit_depth_chroma_minus8;
+
+  pic_param.seq_fields.bits.chroma_format_idc = sps->chroma_format_idc;
+  pic_param.seq_fields.bits.residual_colour_transform_flag =
+      sps->separate_colour_plane_flag;
+  pic_param.seq_fields.bits.gaps_in_frame_num_value_allowed_flag =
+      sps->gaps_in_frame_num_value_allowed_flag;
+  pic_param.seq_fields.bits.frame_mbs_only_flag =
+      sps->frame_mbs_only_flag;
+  pic_param.seq_fields.bits.mb_adaptive_frame_field_flag =
+      sps->mb_adaptive_frame_field_flag;
+  pic_param.seq_fields.bits.direct_8x8_inference_flag =
+      sps->direct_8x8_inference_flag;
+  pic_param.seq_fields.bits.MinLumaBiPredSize8x8 =
+      (sps->level_idc >= 31);
+  pic_param.seq_fields.bits.log2_max_frame_num_minus4 =
+      sps->log2_max_frame_num_minus4;
+  pic_param.seq_fields.bits.pic_order_cnt_type =
+      sps->pic_order_cnt_type;
+  pic_param.seq_fields.bits.log2_max_pic_order_cnt_lsb_minus4 =
+      sps->log2_max_pic_order_cnt_lsb_minus4;
+  pic_param.seq_fields.bits.delta_pic_order_always_zero_flag =
+      sps->delta_pic_order_always_zero_flag;
+
+  pic_param.num_slice_groups_minus1 = pps->num_slice_groups_minus1;
+  pic_param.slice_group_map_type = 0;
+  pic_param.slice_group_change_rate_minus1 = 0;
+  pic_param.pic_init_qp_minus26 = pps->pic_init_qp_minus26;
+  pic_param.pic_init_qs_minus26 = pps->pic_init_qs_minus26;
+  pic_param.chroma_qp_index_offset = pps->chroma_qp_index_offset;
+  pic_param.second_chroma_qp_index_offset =
+      pps->second_chroma_qp_index_offset;
+
+  pic_param.pic_fields.bits.entropy_coding_mode_flag =
+      pps->entropy_coding_mode_flag;
+  pic_param.pic_fields.bits.weighted_pred_flag = pps->weighted_pred_flag;
+  pic_param.pic_fields.bits.weighted_bipred_idc = pps->weighted_bipred_idc;
+  pic_param.pic_fields.bits.transform_8x8_mode_flag =
+      pps->transform_8x8_mode_flag;
+
+  pic_param.pic_fields.bits.field_pic_flag = 0;
+  pic_param.pic_fields.bits.constrained_intra_pred_flag =
+      pps->constrained_intra_pred_flag;
+  pic_param.pic_fields.bits.pic_order_present_flag =
+      pps->bottom_field_pic_order_in_frame_present_flag;
+  pic_param.pic_fields.bits.deblocking_filter_control_present_flag =
+      pps->deblocking_filter_control_present_flag;
+  pic_param.pic_fields.bits.redundant_pic_cnt_present_flag =
+      pps->redundant_pic_cnt_present_flag;
+  pic_param.pic_fields.bits.reference_pic_flag = curr_pic_->ref;
+
+  pic_param.frame_num = curr_pic_->frame_num;
+
+  InitVAPicture(&pic_param.CurrPic);
+  FillVAPicture(&pic_param.CurrPic, curr_pic_.get());
+
+  // Init reference pictures' array.
+  for (int i = 0; i < 16; ++i)
+    InitVAPicture(&pic_param.ReferenceFrames[i]);
+
+  // And fill it with picture info from DPB.
+  FillVARefFramesFromDPB(pic_param.ReferenceFrames);
+
+  pic_param.num_ref_frames = sps->max_num_ref_frames;
+
+  // Allocate a buffer in driver for this parameter buffer and upload data.
+  VAStatus va_status = vaapi_vaCreateBuffer(va_display_, va_context_id_,
+            VAPictureParameterBufferType, sizeof(VAPictureParameterBufferH264),
+            1, &pic_param, &pic_param_buf_id);
+  VA_SUCCESS_OR_RETURN(va_status, "Failed to create a buffer for PPS", false);
+
+  // Queue its VA buffer ID to be committed on HW decode run.
+  pending_va_bufs_.push(pic_param_buf_id);
+
+  return true;
+}
+
+// Fill a VAIQMatrixBufferH264 to be later sent to the HW decoder.
+bool VaapiH264Decoder::SendIQMatrix() {
+  VAIQMatrixBufferH264 iq_matrix_buf;
+  VABufferID iq_matrix_buf_id;
+  int i, j;
+
+  H264PPS* pps = parser_.GetPPS(curr_pps_id_);
+  DCHECK(pps);
+
+  memset(&iq_matrix_buf, 0, sizeof(VAIQMatrixBufferH264));
+
+  if (pps->pic_scaling_matrix_present_flag) {
+    for (i = 0; i < 6; ++i)
+      for (j = 0; j < 16; ++j)
+        iq_matrix_buf.ScalingList4x4[i][j] = pps->scaling_list4x4[i][j];
+
+    for (i = 0; i < 2; ++i)

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

I'm surprised by the '2' here and at l.793, expecting to see 6's.

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

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

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

Are the other 4 rows left untouched??

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

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.

+      for (j = 0; j < 64; ++j)
+        iq_matrix_buf.ScalingList8x8[i][j] = pps->scaling_list8x8[i][j];
+  } else {
+    H264SPS* sps = parser_.GetSPS(pps->seq_parameter_set_id);
+    DCHECK(sps);
+    for (i = 0; i < 6; ++i)
+      for (j = 0; j < 16; ++j)
+        iq_matrix_buf.ScalingList4x4[i][j] = sps->scaling_list4x4[i][j];
+
+    for (i = 0; i < 2; ++i)
+      for (j = 0; j < 64; ++j)
+        iq_matrix_buf.ScalingList8x8[i][j] = sps->scaling_list8x8[i][j];
+  }
+
+  // Allocate a buffer in driver for this parameter buffer and upload data.
+  VAStatus va_status = vaapi_vaCreateBuffer(va_display_, va_context_id_,
+                  VAIQMatrixBufferType, sizeof(VAIQMatrixBufferH264), 1,
+                  &iq_matrix_buf, &iq_matrix_buf_id);
+  VA_SUCCESS_OR_RETURN(va_status, "Failed to create a buffer for IQMatrix",
+                       false);
+
+  // Queue its VA buffer ID to be committed on HW decode run.
+  pending_va_bufs_.push(iq_matrix_buf_id);
+
+  return true;
+}
+
+bool VaapiH264Decoder::SendVASliceParam(H264SliceHeader* slice_hdr) {
+  VASliceParameterBufferH264 slice_param;
+  VABufferID slice_param_buf_id;
+  VAStatus va_status;
+  int i;
+
+  H264PPS* pps = parser_.GetPPS(slice_hdr->pic_parameter_set_id);
+  DCHECK(pps);
+
+  H264SPS* sps = parser_.GetSPS(pps->seq_parameter_set_id);
+  DCHECK(sps);
+
+  memset(&slice_param, 0, sizeof(VASliceParameterBufferH264));
+
+  slice_param.slice_data_size = slice_hdr->nalu_size;
+  slice_param.slice_data_offset = 0;
+  slice_param.slice_data_flag = VA_SLICE_DATA_FLAG_ALL;
+  slice_param.slice_data_bit_offset = slice_hdr->header_bit_size;
+
+  slice_param.first_mb_in_slice = slice_hdr->first_mb_in_slice;
+  slice_param.slice_type = slice_hdr->slice_type % 5;
+  slice_param.direct_spatial_mv_pred_flag =
+      slice_hdr->direct_spatial_mv_pred_flag;
+
+  // TODO posciak: make sure parser sets those even when override flags
+  // in slice header is off.
+  slice_param.num_ref_idx_l0_active_minus1 =
+      slice_hdr->num_ref_idx_l0_active_minus1;
+  slice_param.num_ref_idx_l1_active_minus1 =
+      slice_hdr->num_ref_idx_l1_active_minus1;
+  slice_param.cabac_init_idc = slice_hdr->cabac_init_idc;
+
+  slice_param.slice_qp_delta = slice_hdr->slice_qp_delta;
+  slice_param.disable_deblocking_filter_idc =
+      slice_hdr->disable_deblocking_filter_idc;
+
+  slice_param.slice_alpha_c0_offset_div2 =
+      slice_hdr->slice_alpha_c0_offset_div2;
+  slice_param.slice_beta_offset_div2 = slice_hdr->slice_beta_offset_div2;
+
+  if (((IsH264PSlice(slice_hdr) || IsH264SPSlice(slice_hdr))
+        && pps->weighted_pred_flag)
+      || (IsH264BSlice(slice_hdr) && pps->weighted_bipred_idc == 1)) {
+    slice_param.luma_log2_weight_denom = slice_hdr->luma_log2_weight_denom;
+    slice_param.chroma_log2_weight_denom = slice_hdr->chroma_log2_weight_denom;
+
+    slice_param.luma_weight_l0_flag = slice_hdr->luma_weight_l0_flag;
+    slice_param.luma_weight_l1_flag = slice_hdr->luma_weight_l1_flag;
+
+    slice_param.chroma_weight_l0_flag = slice_hdr->chroma_weight_l0_flag;
+    slice_param.chroma_weight_l1_flag = slice_hdr->chroma_weight_l1_flag;
+
+    for (i = 0; i <= slice_param.num_ref_idx_l0_active_minus1; ++i) {
+      slice_param.luma_weight_l0[i] =
+          slice_hdr->pred_weight_table_l0.luma_weight[i];
+      slice_param.luma_offset_l0[i] =
+          slice_hdr->pred_weight_table_l0.luma_offset[i];
+    }
+
+    for (i = 0; i <= slice_param.num_ref_idx_l0_active_minus1; ++i) {
+      slice_param.chroma_weight_l0[i][0] =
+          slice_hdr->pred_weight_table_l0.chroma_weight[i][0];
+      slice_param.chroma_weight_l0[i][1] =
+          slice_hdr->pred_weight_table_l0.chroma_weight[i][1];
+      slice_param.chroma_offset_l0[i][0] =
+          slice_hdr->pred_weight_table_l0.chroma_offset[i][0];
+      slice_param.chroma_offset_l0[i][1] =
+          slice_hdr->pred_weight_table_l0.chroma_offset[i][1];
+    }
+
+    if (IsH264BSlice(slice_hdr)) {
+      for (i = 0; i <= slice_param.num_ref_idx_l1_active_minus1; ++i) {
+        slice_param.luma_weight_l1[i] =
+            slice_hdr->pred_weight_table_l1.luma_weight[i];
+        slice_param.luma_offset_l1[i] =
+            slice_hdr->pred_weight_table_l1.luma_offset[i];
+      }
+
+      for (i = 0; i <= slice_param.num_ref_idx_l1_active_minus1; ++i) {
+        slice_param.chroma_weight_l1[i][0] =
+            slice_hdr->pred_weight_table_l1.chroma_weight[i][0];
+        slice_param.chroma_weight_l1[i][1] =
+            slice_hdr->pred_weight_table_l1.chroma_weight[i][1];
+        slice_param.chroma_offset_l1[i][0] =
+            slice_hdr->pred_weight_table_l1.chroma_offset[i][0];
+        slice_param.chroma_offset_l1[i][1] =
+            slice_hdr->pred_weight_table_l1.chroma_offset[i][1];
+      }
+    }
+  }
+
+  for (i = 0; i < 32; ++i) {
+    InitVAPicture(&slice_param.RefPicList0[i]);
+    InitVAPicture(&slice_param.RefPicList1[i]);
+  }
+
+  PicsVector::iterator it;
+  for (it = RefPicList0_.begin(), i = 0; it != RefPicList0_.end(); ++it, ++i)
+    FillVAPicture(&slice_param.RefPicList0[i], *it);
+
+  for (it = RefPicList1_.begin(), i = 0; it != RefPicList1_.end(); ++it, ++i)
+    FillVAPicture(&slice_param.RefPicList1[i], *it);
+
+  // Allocate a buffer in driver for this parameter buffer and upload data.
+  va_status = vaapi_vaCreateBuffer(va_display_, va_context_id_,
+                VASliceParameterBufferType, sizeof(VASliceParameterBufferH264),
+                1, &slice_param, &slice_param_buf_id);
+  VA_SUCCESS_OR_RETURN(va_status, "Failed creating a buffer for slice param",
+                       false);
+
+  // Queue its VA buffer ID to be committed on HW decode run.
+  pending_slice_bufs_.push(slice_param_buf_id);
+
+  return true;
+}
+
+bool VaapiH264Decoder::SendSliceData(void* ptr, size_t size)
+{
+    VABufferID slice_data_buf_id;
+    VAStatus va_status;
+
+    va_status = vaapi_vaCreateBuffer(va_display_, va_context_id_,
+                    VASliceDataBufferType, size, 1, ptr, &slice_data_buf_id);
+    VA_SUCCESS_OR_RETURN(va_status, "Failed creating a buffer for slice data",
+                         false);
+
+    pending_slice_bufs_.push(slice_data_buf_id);
+
+    return true;
+}
+
+bool VaapiH264Decoder::QueueSlice(H264SliceHeader* slice_hdr) {
+  DCHECK(curr_pic_.get());
+
+  if (!SendVASliceParam(slice_hdr))
+    return false;
+
+  if (!SendSliceData(slice_hdr->nalu_data, slice_hdr->nalu_size))
+    return false;
+
+  return true;
+}
+
+// TODO posciak: start using vaMapBuffer instead of vaCreateBuffer wherever

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

What's your plan here?

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

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

+// possible.
+
+bool VaapiH264Decoder::DecodePicture() {
+  static const size_t kMaxVABuffers = 32;
+  VABufferID buffers[kMaxVABuffers];
+  VAStatus va_status;
+
+  DCHECK(!frame_decoded_);
+  DCHECK(curr_pic_.get());
+
+  DCHECK_LE(pending_va_bufs_.size(), kMaxVABuffers);
+  DCHECK_LE(pending_slice_bufs_.size(), kMaxVABuffers);
+
+  DVLOG(4) << "Pending VA bufs to commit: " << pending_va_bufs_.size();
+  DVLOG(4) << "Pending slice bufs to commit: " << pending_slice_bufs_.size();
+
+  // Find the surface associated with the picture to be decoded.
+  DCHECK(pending_slice_bufs_.size());
+  DecodeSurface* dec_surface = poc_to_decode_surfaces_[curr_pic_->PicOrderCnt];
+  DVLOG(4) << "Decoding POC " << curr_pic_->PicOrderCnt
+           << " into surface " << dec_surface->va_surface_id;
+
+  // Get ready to decode into surface.
+  va_status = vaapi_vaBeginPicture(va_display_, va_context_id_,
+                                   dec_surface->va_surface_id);
+  VA_SUCCESS_OR_RETURN(va_status, "vaBeginPicture failed", false);
+
+  // Put buffer IDs for pending parameter buffers into buffers[].
+  size_t num_buffers = pending_va_bufs_.size();
+  for (size_t i = 0; i < num_buffers && i < kMaxVABuffers; ++i) {
+    buffers[i] = pending_va_bufs_.front();

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

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?

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

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

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

If that's the problem, I'm not seeing a solution :)
It is indeed negligble.
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.

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

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

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

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

+    pending_va_bufs_.pop();
+  }
+
+  // And send them to the HW decoder.
+  va_status = vaapi_vaRenderPicture(va_display_, va_context_id_, buffers,
+                                    num_buffers);
+  VA_SUCCESS_OR_RETURN(va_status, "vaRenderPicture for va_bufs failed", false);
+
+  DVLOG(4) << "Committed " << num_buffers << "VA buffers";
+
+  // Put buffer IDs for pending slice data buffers into buffers[].
+  num_buffers = pending_slice_bufs_.size();
+  for (size_t i = 0; i < num_buffers && i < kMaxVABuffers; ++i) {
+    buffers[i] = pending_slice_bufs_.front();
+    pending_slice_bufs_.pop();
+  }
+
+  // And send them to the Hw decoder.
+  va_status = vaapi_vaRenderPicture(va_display_, va_context_id_, buffers,
+                                    num_buffers);
+  VA_SUCCESS_OR_RETURN(va_status, "vaRenderPicture fo slices failed", false);
+
+  DVLOG(4) << "Committed " << num_buffers << "slice buffers";
+
+  // Instruct HW decoder to start processing committed buffers (decode this
+  // picture). This does not block until the end of decode.
+  va_status = vaapi_vaEndPicture(va_display_, va_context_id_);
+  VA_SUCCESS_OR_RETURN(va_status, "vaEndPicture failed", false);
+
+  // Used to notify clients that we had sufficient data to start decoding
+  // a new frame.
+  frame_decoded_ = true;

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

this var name and the comment preceding it are at odds. 
At this point in time has the frame been decoded or not?

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

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.

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

Then s/frame_decoded_/frame_ready_at_hw_/

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

Done.

+
+  return true;
+}
+
+
+bool VaapiH264Decoder::InitCurrPicture(H264SliceHeader* slice_hdr) {
+  DCHECK(curr_pic_.get());
+
+  memset(curr_pic_.get(), 0, sizeof(H264Picture));
+
+  curr_pic_->idr = slice_hdr->idr_pic_flag;
+
+  if (slice_hdr->field_pic_flag)
+    curr_pic_->field = slice_hdr->bottom_field_flag ? H264Picture::FIELD_BOTTOM

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

multi-line then clause requires braces for then & else clauses.

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

Done.

+                                                    : H264Picture::FIELD_TOP;
+  else
+    curr_pic_->field = H264Picture::FIELD_NONE;
+
+  curr_pic_->ref = slice_hdr->nal_ref_idc != 0;
+  // Not correct if not a field.

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

not sure what this means.

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

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.

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

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

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

Done.

+  curr_pic_->frame_num = curr_pic_->PicNum = slice_hdr->frame_num;
+
+  if (!CalculatePicOrderCounts(slice_hdr))
+    return false;
+
+  // Try to get an empty surface to decode this picture to.
+  if (!AssignSurfaceToPoC(curr_pic_->PicOrderCnt)) {
+    DLOG(INFO) << "Failed getting a free surface for a picture";
+    return false;
+  }
+
+  curr_pic_->long_term_reference_flag = slice_hdr->long_term_reference_flag;
+  curr_pic_->adaptive_ref_pic_marking_mode_flag =
+                    slice_hdr->adaptive_ref_pic_marking_mode_flag;
+
+  // If the slice header indicates we will have to perform reference marking
+  // process after this picture is decoded, store required data for that
+  // purpose.
+  if (slice_hdr->adaptive_ref_pic_marking_mode_flag) {
+    DCHECK_EQ(sizeof(curr_pic_->ref_pic_marking),

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

This is a COMPILE_ASSERT that can go somewhere (not in the decode code-path)?

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

But only in debug build, but you are right.

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

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

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

Can't agree more. Done.

+              sizeof(slice_hdr->ref_pic_marking));
+    memcpy(curr_pic_->ref_pic_marking, slice_hdr->ref_pic_marking,
+            sizeof(curr_pic_->ref_pic_marking));
+  }
+
+  return true;
+}
+
+bool VaapiH264Decoder::CalculatePicOrderCounts(H264SliceHeader* slice_hdr) {
+  int prevPicOrderCntMsb, prevPicOrderCntLsb;
+  int pic_order_cnt_lsb = slice_hdr->pic_order_cnt_lsb;
+
+  DCHECK_NE(curr_sps_id_, -1);
+
+  curr_pic_->pic_order_cnt_lsb = pic_order_cnt_lsb;
+
+  switch (parser_.GetSPS(curr_sps_id_)->pic_order_cnt_type) {

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

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.

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

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

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

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

+    case 0:
+      /* See spec 8.2.1.1 */
+      if (slice_hdr->idr_pic_flag) {
+        prevPicOrderCntMsb = prevPicOrderCntLsb = 0;
+      } else {
+        if (prev_ref_has_memmgmnt5_) {
+          if (prev_ref_field_ != H264Picture::FIELD_BOTTOM) {
+            prevPicOrderCntMsb = 0;
+            prevPicOrderCntLsb = prev_ref_TopFieldOrderCnt_;
+          } else {
+            prevPicOrderCntMsb = 0;
+            prevPicOrderCntLsb = 0;
+          }
+        } else {
+          prevPicOrderCntMsb = prev_ref_PicOrderCntMsb_;
+          prevPicOrderCntLsb = prev_ref_pic_order_cnt_lsb_;
+        }
+      }
+
+      DCHECK_NE(MaxPicOrderCntLsb_, 0);
+      if ((pic_order_cnt_lsb < prevPicOrderCntLsb) &&
+          (prevPicOrderCntLsb - pic_order_cnt_lsb >= MaxPicOrderCntLsb_ / 2)) {
+        curr_pic_->PicOrderCntMsb = prevPicOrderCntMsb + MaxPicOrderCntLsb_;
+      } else if ((pic_order_cnt_lsb > prevPicOrderCntLsb) &&
+          (pic_order_cnt_lsb - prevPicOrderCntLsb > MaxPicOrderCntLsb_ / 2)) {
+        curr_pic_->PicOrderCntMsb = prevPicOrderCntMsb - MaxPicOrderCntLsb_;
+      } else {
+        curr_pic_->PicOrderCntMsb = prevPicOrderCntMsb;
+      }
+
+      if (curr_pic_->field != H264Picture::FIELD_BOTTOM)
+        curr_pic_->TopFieldOrderCnt = curr_pic_->PicOrderCntMsb
+                                    + pic_order_cnt_lsb;
+
+      if (curr_pic_->field != H264Picture::FIELD_TOP) {
+        // TODO posciak: perhaps replace with pic->field?
+        if (!slice_hdr->field_pic_flag)
+          curr_pic_->BottomFieldOrderCnt = curr_pic_->TopFieldOrderCnt
+                                       + slice_hdr->delta_pic_order_cnt_bottom;
+        else
+          curr_pic_->BottomFieldOrderCnt = curr_pic_->PicOrderCntMsb
+                                         + pic_order_cnt_lsb;
+      }
+      break;
+
+    case 1:
+      NOTIMPLEMENTED();
+      return false;
+      break;

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

break is unnecessary after return (here and elsewhere).

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

Done.

+
+    case 2:
+      NOTIMPLEMENTED();
+      return false;
+      break;
+
+    default:
+      NOTREACHED();
+  }
+
+  switch (curr_pic_->field) {
+    case H264Picture::FIELD_NONE:
+      curr_pic_->PicOrderCnt = std::min(curr_pic_->TopFieldOrderCnt,
+                                        curr_pic_->BottomFieldOrderCnt);
+      break;
+

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

extra newlines are unnecessary.

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

Done.

+    case H264Picture::FIELD_TOP:
+      curr_pic_->PicOrderCnt = curr_pic_->TopFieldOrderCnt;
+      break;
+
+    case H264Picture::FIELD_BOTTOM:
+      curr_pic_->PicOrderCnt = curr_pic_->BottomFieldOrderCnt;
+      break;
+
+    default:

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

drop default clause

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

Done.

+      NOTREACHED();
+  }
+
+  return true;
+}
+
+void VaapiH264Decoder::UpdatePicNums() {
+  H264Picture* pic;

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

move to first use at l.1159.

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

Done.

+
+  for (PicsVector::iterator it = dpb_.begin(); it != dpb_.end(); ++it) {
+    pic = *it;
+    DCHECK(pic);
+
+    if (!pic->ref)
+      continue;
+
+    // Below is not correct if a field.

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

unclear

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

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

+    DCHECK_EQ(pic->field, H264Picture::FIELD_NONE);
+    if (pic->long_term) {
+      pic->LongTermPicNum = pic->LongTermFrameIdx;
+    } else {
+      if (pic->frame_num > frame_num_)
+        pic->FrameNumWrap = pic->frame_num - MaxFrameNum_;
+      else
+        pic->FrameNumWrap = pic->frame_num;
+
+      pic->PicNum = pic->FrameNumWrap;
+    }
+  }
+}
+
+void VaapiH264Decoder::ConstructReferencePicListsP(H264SliceHeader* slice_hdr) {
+  // RefPicList0:
+  // shortterm ref pics sorted by descending PicNum,
+  // followed by long term ref pics by ascending LongTermPicNum.
+
+  DCHECK(RefPicList0_.empty() && RefPicList1_.empty());
+  dpb_.GetShortTermRefPicsAppending(RefPicList0_);
+  size_t num_short_refs = RefPicList0_.size();
+  std::sort(RefPicList0_.begin(), RefPicList0_.end(),

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

This (and below) is an awful lot of std::sort()'ing.
Is it really necessary?

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

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?

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

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?

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

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

+            H264Picture::PicNumDescCompare());
+  dpb_.GetLongTermRefPicsAppending(RefPicList0_);
+  std::sort(RefPicList0_.begin() + num_short_refs, RefPicList0_.end(),
+            H264Picture::LongTermPicNumAscCompare());
+
+  //DCHECK((int)RefPicList0_.size() <=

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

drop

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

Done.

+            //slice_hdr->num_ref_idx_l0_active_minus1 + 1);

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

drop

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

Done.

+  RefPicList0_.resize(slice_hdr->num_ref_idx_l0_active_minus1 + 1);
+}
+
+void VaapiH264Decoder::ConstructReferencePicListsB(H264SliceHeader* slice_hdr) {

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

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)

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

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. 

+  PicsVector::iterator iter;
+
+  // RefPicList0 (8.2.4.2.3) [[1] [2] [3]], where:
+  // [1] shortterm ref pics with POC < curr_pic's POC sorted by descending POC,
+  // [2] shortterm ref pics with POC > curr_pic's POC by ascending POC,
+  // [3] longterm ref pics by ascending LongTermPicNum.
+
+  DCHECK(RefPicList0_.empty() && RefPicList1_.empty());
+  dpb_.GetShortTermRefPicsAppending(RefPicList0_);
+  size_t num_short_refs = RefPicList0_.size();
+
+  // First sort ascending, this will put [1] in right place and finish [2].
+  std::sort(RefPicList0_.begin(), RefPicList0_.end(),
+            H264Picture::POCAscCompare());
+
+  // Find first with POC > curr_pic's POC to get first element in [2]...
+  iter = std::upper_bound(RefPicList0_.begin(), RefPicList0_.end(),
+                          curr_pic_.get(), H264Picture::POCAscCompare());
+
+  // and sort [1] descending, thus finishing sequence [1] [2].
+  std::sort(RefPicList0_.begin(), iter, H264Picture::POCDescCompare());
+
+  // Now add [3] and sort by ascending LongTermPicNum.
+  dpb_.GetLongTermRefPicsAppending(RefPicList0_);
+  std::sort(RefPicList0_.begin() + num_short_refs, RefPicList0_.end(),
+            H264Picture::LongTermPicNumAscCompare());
+
+
+  // RefPicList1 (8.2.4.2.4) [[1] [2] [3]], where:
+  // [1] shortterm ref pics with POC > curr_pic's POC sorted by ascending POC,
+  // [2] shortterm ref pics with POC < curr_pic's POC by descending POC,
+  // [3] longterm ref pics by ascending LongTermPicNum.
+
+  dpb_.GetShortTermRefPicsAppending(RefPicList1_);
+  num_short_refs = RefPicList1_.size();
+
+  // First sort by descending POC.
+  std::sort(RefPicList1_.begin(), RefPicList1_.end(),
+            H264Picture::POCDescCompare());
+
+  // Find first with POC < curr_pic's POC to get first element in [2]...
+  iter = std::upper_bound(RefPicList1_.begin(), RefPicList1_.end(),
+                          curr_pic_.get(), H264Picture::POCDescCompare());
+
+  // and sort [1] ascending.
+  std::sort(RefPicList1_.begin(), iter, H264Picture::POCAscCompare());
+
+  // Now add [3] and sort by ascending LongTermPicNum
+  dpb_.GetShortTermRefPicsAppending(RefPicList1_);
+  std::sort(RefPicList1_.begin() + num_short_refs, RefPicList1_.end(),
+            H264Picture::LongTermPicNumAscCompare());
+
+  // If lists identical, swap first two entries in RefPicList1 (spec 8.2.4.2.3)
+  if (RefPicList1_.size() > 1 &&
+          std::equal(RefPicList0_.begin(), RefPicList0_.end(),
+                     RefPicList1_.begin()))
+      std::swap(RefPicList1_[0], RefPicList1_[1]);
+
+  //DCHECK((int)RefPicList0_.size() <=
+              //slice_hdr->num_ref_idx_l0_active_minus1 + 1);
+  //DCHECK((int)RefPicList1_.size() <=
+              //slice_hdr->num_ref_idx_l1_active_minus1 + 1);
+
+  // Per 8.2.4.2 it's possible for num_ref_idx_lX_active_minus1 to indicate
+  // there should be more ref pics on list than we constructed.
+  // Those superfluous ones should be treated as non-reference.
+  RefPicList0_.resize(slice_hdr->num_ref_idx_l0_active_minus1 + 1);
+  RefPicList1_.resize(slice_hdr->num_ref_idx_l1_active_minus1 + 1);
+}
+
+// See 8.2.4
+int VaapiH264Decoder::PicNumF(H264Picture *pic) {
+  if (!pic)

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

when can this happen?

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

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.

+      return -1;
+
+  if (!pic->long_term)
+      return pic->PicNum;
+  else
+      return MaxPicNum_;
+}
+
+// See 8.2.4
+int VaapiH264Decoder::LongTermPicNumF(H264Picture *pic) {
+    if (H264Picture::IsLongTermRefPic()(pic))
+        return pic->LongTermPicNum;
+    else
+        return 2 * (MaxLongTermFrameIdx_ + 1);
+}
+
+bool VaapiH264Decoder::ModifyReferencePicList(H264SliceHeader *slice_hdr,
+                                              int list) {
+  int i;

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

Please declare variables at the latest possible time.
(here and elsewhere; please make a pass)

+
+  int refIdxLX = 0;
+  int picNumLXNoWrap;
+  int picNumLX;
+  int picNumLXPred = curr_pic_->PicNum;
+  int cIdx, nIdx;
+  int num_ref_idx_lX_active_minus1;
+
+  PicsVector* RefPicListX;
+  H264ModificationOfPicNum* list_mod;
+
+  // Set up pointers to proper list to be processed.
+  if (list == 0) {
+    if (!slice_hdr->ref_pic_list_modification_flag_l0)
+      return true;
+
+    list_mod = slice_hdr->ref_list_l0_modifications;
+    num_ref_idx_lX_active_minus1 = RefPicList0_.size() - 1;
+
+    RefPicListX = &RefPicList0_;
+  } else {
+    if (!slice_hdr->ref_pic_list_modification_flag_l1)
+      return true;
+
+    list_mod = slice_hdr->ref_list_l1_modifications;
+    num_ref_idx_lX_active_minus1 = RefPicList1_.size() - 1;
+
+    RefPicListX = &RefPicList1_;
+  }
+
+  DCHECK_GT(num_ref_idx_lX_active_minus1, 0);
+
+  // Spec 8.2.4.3:
+  // Reorder pictures on the list in a way specified in the stream.
+  for (i = 0; i < H264SliceHeader::kRefListModSize; ++i) {
+    switch (list_mod->modification_of_pic_nums_idc) {
+      case 0:
+      case 1:
+        // Modify short reference picture position.
+        if (list_mod->modification_of_pic_nums_idc == 0) {
+          if (picNumLXPred - ((int)list_mod->abs_diff_pic_num_minus1 + 1)
+                < 0)
+            picNumLXNoWrap = picNumLXPred
+                           - ((int)list_mod->abs_diff_pic_num_minus1 + 1)

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

indent is wrong here & elsewhere
operators belong on preceding line, always.

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

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

+                           + MaxPicNum_;
+          else
+            picNumLXNoWrap = picNumLXPred
+                           - ((int)list_mod->abs_diff_pic_num_minus1 + 1);
+        } else {
+          if (picNumLXPred + ((int)list_mod->abs_diff_pic_num_minus1 + 1)
+                >= MaxPicNum_)
+            picNumLXNoWrap = picNumLXPred
+                           + ((int)list_mod->abs_diff_pic_num_minus1 + 1)
+                           - MaxPicNum_;
+          else
+            picNumLXNoWrap = picNumLXPred
+                           + ((int)list_mod->abs_diff_pic_num_minus1 + 1);
+        }
+
+        picNumLXPred = picNumLXNoWrap;
+
+        if (picNumLXNoWrap > curr_pic_->PicNum)
+          picNumLX = picNumLXNoWrap - MaxPicNum_;
+        else
+          picNumLX = picNumLXNoWrap;
+
+        DCHECK_LT(num_ref_idx_lX_active_minus1 + 1,
+                  H264SliceHeader::kRefListModSize);
+
+        // Shift all pictures starting from refIdxLX one position to the right.
+        for (cIdx = num_ref_idx_lX_active_minus1 + 1; cIdx > refIdxLX; --cIdx)
+          (*RefPicListX)[cIdx] = (*RefPicListX)[cIdx - 1];
+
+        // Put picNumLX at refIdxLX.
+        (*RefPicListX)[refIdxLX++] = dpb_.GetShortRefPicByPicNum(picNumLX);
+        DCHECK((*RefPicListX)[refIdxLX - 1]);
+
+        // Now shift the pictures after its original position.
+        nIdx = refIdxLX;
+        for (cIdx = refIdxLX;
+             cIdx <= num_ref_idx_lX_active_minus1 + 1; ++cIdx) {
+          if (PicNumF((*RefPicListX)[cIdx]) != picNumLX)
+            (*RefPicListX)[nIdx++] = (*RefPicListX)[cIdx];
+        }
+        break;
+
+      case 2:
+        // Modify long term reference picture position.
+        DCHECK_LT(num_ref_idx_lX_active_minus1 + 1,
+                  H264SliceHeader::kRefListModSize);
+
+        // Shift all pictures starting from refIdxLX to the right.

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

Can you try to extract common functionality into helper functions to avoid
duplicate structures?

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

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

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

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

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

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.

+        for (cIdx = num_ref_idx_lX_active_minus1 + 1; cIdx > refIdxLX; --cIdx)
+          (*RefPicListX)[cIdx] = (*RefPicListX)[cIdx - 1];
+
+        // Put given LongTermPicNum at refIdxLX.
+        (*RefPicListX)[refIdxLX++] =
+          dpb_.GetLongRefPicByLongTermPicNum(list_mod->long_term_pic_num);
+        DCHECK((*RefPicListX)[refIdxLX - 1]);
+
+        nIdx = refIdxLX;
+
+        // Now shift the pictures after its original position.
+        for (cIdx = refIdxLX; cIdx <= num_ref_idx_lX_active_minus1 + 1; ++cIdx)
+          if (LongTermPicNumF((*RefPicListX)[cIdx])
+                != curr_pic_->LongTermPicNum)
+            (*RefPicListX)[nIdx++] = (*RefPicListX)[cIdx];
+        break;
+
+      case 3:
+        // End of modification list.
+        return true;
+
+      default:
+        // May be recoverable.
+        DLOG(INFO) << "Invalid modification_of_pic_nums_idc="
+                   << list_mod->modification_of_pic_nums_idc
+                   << " in position " << i;
+        break;
+    }
+
+    ++list_mod;
+  }
+
+  return true;
+}
+
+// Must be run in GLXContext thread.

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

Replace this comment with a DCHECK at the top of the function.

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

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.

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

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?

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

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

+bool VaapiH264Decoder::PutPicToTexture(int32 picture_buffer_id) {
+  DecodeSurfaces::iterator it = decode_surfaces_.find(picture_buffer_id);
+  if (it == decode_surfaces_.end()) {
+    DLOG(ERROR) << "Asked to put an invalid buffer";
+    return false;
+  }
+
+  DVLOG(3) << "Will output from VASurface " << it->second->va_surface_id
+           << " to texture id " << it->second->texture_id;
+
+  TrapX11Errors();
+
+  // Put the decoded data into XPixmap bound to the texture.

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

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?

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

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.

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

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.
Of course; but why are you bringing up SW decode?

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

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

+  VAStatus va_status = vaapi_vaPutSurface(va_display_,
+      it->second->va_surface_id, it->second->x_pixmap,
+      0, 0, pic_width_, pic_height_,
+      0, 0, pic_width_, pic_height_,
+      NULL, 0, 0);
+  VA_SUCCESS_OR_RETURN(va_status,
+                       "Failed putting decoded picture to texture", false);
+  // TODO posciak: needed?
+  XSync(x_display_, False);
+
+  // Wait for the data to be put into the buffer so it'd ready for output.
+  va_status = vaapi_vaSyncSurface(va_display_, it->second->va_surface_id);
+  VA_SUCCESS_OR_RETURN(va_status, "Failed syncing decoded picture", false);
+
+  UntrapX11Errors();
+  return true;
+}
+
+bool VaapiH264Decoder::OutputPic(H264Picture* pic) {
+  DCHECK(poc_to_decode_surfaces_.find(pic->PicOrderCnt) !=
+         poc_to_decode_surfaces_.end());
+
+  // Notify client that a picture is ready to be outputted. This does not
+  // require the data to be synced with texture contents. Client must use

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

Why do it this way?

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

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.

+  // PutPicToTexture() to ensure that.
+  DecodeSurface* dec_surface = poc_to_decode_surfaces_[pic->PicOrderCnt];
+  DCHECK(dec_surface);
+
+  DVLOG(4) << "Posting output task for input_id: " << dec_surface->input_id
+           << "output_id: " << dec_surface->picture_buffer_id;
+
+  DCHECK(msg_loop_);

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

I thought you were going to PostTask to this msg loop, not DCHECK it here.

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

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?

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

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.

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

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.

+  output_pic_callback_(output_pic_callback_arg_, dec_surface->input_id,
+                       dec_surface->picture_buffer_id);
+  return true;
+}
+
+bool VaapiH264Decoder::Flush() {
+  PicsVector to_output;
+  PicsVector::iterator it;
+
+  // Output all pictures that are waiting to be outputted.
+  // Sort by POC before outputting
+  dpb_.GetNotOutputtedPicsSortedByPOCAppending(to_output);
+  for (it = to_output.begin(); it != to_output.end(); ++it) {
+    if (!OutputPic(*it)) {
+      DLOG(ERROR) << "Failed to output pic POC: " << (*it)->PicOrderCnt;
+      return false;
+    }
+  }
+
+  // And clear DPB contents.
+  dpb_.Clear();
+
+  return true;
+}
+
+bool VaapiH264Decoder::StartNewFrame(H264SliceHeader* slice_hdr) {
+// TODO posciak: add handling of max_num_ref_frames per spec.
+
+  // If the new frame is an IDR, output what's left to output and clear DPB
+  if (slice_hdr->idr_pic_flag) {
+    // (unless we are explicitly instructed not to do so).
+    if (!slice_hdr->no_output_of_prior_pics_flag) {
+      // Output DPB contents.
+      if (!Flush())
+        return false;
+    }
+    dpb_.Clear();
+  }
+
+  // curr_pic_ should have either been added to DPB or discarded when finishing
+  // the last frame. DPB is responsible for releasing that memory once it's
+  // not needed anymore.
+  DCHECK(!curr_pic_.get());
+  curr_pic_.reset(new H264Picture);
+  CHECK(curr_pic_.get());
+
+  if (!InitCurrPicture(slice_hdr))
+    return false;
+
+  DCHECK_GT(MaxFrameNum_, 0);
+
+  UpdatePicNums();
+
+  // Prepare reference picture lists if required (B and S/SP slices).

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

comment is conditional but next two lines aren't.

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

Yeah, it refers to the ifs below as well.

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

Then remove the newline at 1521.

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

Done.

+  RefPicList0_.clear();
+  RefPicList1_.clear();
+
+  if (IsH264PSlice(slice_hdr) || IsH264SPSlice(slice_hdr)) {
+    DVLOG(4) << "================ P/SP slice, POC= " << curr_pic_->PicOrderCnt
+             << " PicNum= " << curr_pic_->PicNum;
+
+    ConstructReferencePicListsP(slice_hdr);
+    ModifyReferencePicList(slice_hdr, 0);
+

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

lots of unnecessary \n's IMO

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

Done.

+  } else if (IsH264BSlice(slice_hdr)) {
+    DVLOG(4) << "================ B slice, POC= " << curr_pic_->PicOrderCnt

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

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.

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

Right, those ones can go easily.

+             << " PicNum= " << curr_pic_->PicNum;
+
+    ConstructReferencePicListsB(slice_hdr);
+    ModifyReferencePicList(slice_hdr, 0);
+    ModifyReferencePicList(slice_hdr, 1);
+
+  } else {
+    DVLOG(4) << "================ I slice, POC= " << curr_pic_->PicOrderCnt
+             << " PicNum= " << curr_pic_->PicNum;
+  }
+
+  // Send parameter buffers before each new picture, before the first slice.
+  if (!SendPPS())
+    return false;
+
+  if (!SendIQMatrix())
+    return false;
+
+  if (!QueueSlice(slice_hdr))
+    return false;
+
+  return true;
+}
+
+struct MarkLongTermPicUnusedForRefIfOverMax

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

ditto my comment about explicit loops over algorithms here and below.

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

Done.

+    : public std::binary_function<H264Picture*, int, void> {
+
+  // Marks all long term pictures, for which their LongTermFrameIdx is
+  // over given |max_long_term_frame_idx|, as unused.
+  void operator()(H264Picture* pic, const int& max_long_term_frame_idx) const {
+    DCHECK(pic->ref && pic->long_term);
+    if (pic->LongTermFrameIdx > max_long_term_frame_idx)
+      pic->ref = false;
+  }
+};
+
+struct MarkLongTermPicUnusedForRefByLongTermFrameIdx
+    : public std::binary_function<H264Picture*, int, void> {
+
+  // Marks one long term frame picture, for which its LongTermFrameIdx is
+  // over |long_term_frame_idx|.
+  void operator()(H264Picture* pic, const int &long_term_frame_idx) const {
+    DCHECK(pic->ref && pic->long_term);
+    if (pic->LongTermFrameIdx == long_term_frame_idx)
+      pic->ref = false;
+  }
+};
+
+bool VaapiH264Decoder::HandleMemoryManagementOps() {
+  H264DecRefPicMarking *ref_pic_marking;
+  int picNumX;
+  H264Picture* to_mark;
+
+  // 8.2.5.4
+  for (unsigned int i = 0; i < arraysize(curr_pic_->ref_pic_marking); ++i) {
+    // Code below does not support field pictures.
+    ref_pic_marking = &curr_pic_->ref_pic_marking[i];
+
+    switch (ref_pic_marking->memory_mgmnt_control_operation) {
+      case 0:
+        // Normal end of operations' specification.
+        return true;
+
+      case 1:
+        // Mark a short term reference picture as unused so it can be removed
+        // if outputted.
+        picNumX = curr_pic_->PicNum
+                - (ref_pic_marking->difference_of_pic_nums_minus1 + 1);
+        to_mark = dpb_.GetShortRefPicByPicNum(picNumX);
+        if (to_mark) {
+          to_mark->ref = false;
+        } else {
+          DLOG(ERROR) << "Invalid short ref pic num to unmark";
+          return false;
+        }
+        break;
+
+      case 2:
+        // Mark a long term reference picture as unused so it can be removed
+        // if outputted.
+        to_mark = dpb_.GetLongRefPicByLongTermPicNum(
+                                          ref_pic_marking->long_term_pic_num);
+        if (to_mark) {
+          to_mark->ref = false;
+        } else {
+          DLOG(ERROR) << "Invalid long term ref pic num to unmark";
+          return false;
+        }
+        break;
+
+      case 3:
+        // Mark a short term reference picture as long term reference.
+        picNumX = curr_pic_->PicNum
+                - (ref_pic_marking->difference_of_pic_nums_minus1 + 1);
+        to_mark = dpb_.GetShortRefPicByPicNum(picNumX);
+        if (to_mark) {
+          DCHECK(to_mark->ref && !to_mark->long_term);
+          to_mark->long_term = true;
+          to_mark->LongTermFrameIdx = ref_pic_marking->long_term_frame_idx;
+        } else {
+          DLOG(ERROR) << "Invalid short term ref pic num to mark as long ref";
+          return false;
+        }
+        break;
+
+      case 4: {
+        // Unmark all reference pictures with LongTermFrameIdx over new max.
+        MaxLongTermFrameIdx_ = ref_pic_marking->max_long_term_frame_idx_plus1
+                             - 1;
+        PicsVector long_terms;
+        dpb_.GetLongTermRefPicsAppending(long_terms);
+        for_each(long_terms.begin(), long_terms.end(),
+                 std::bind2nd(MarkLongTermPicUnusedForRefIfOverMax(),
+                              MaxLongTermFrameIdx_));
+        break;
+      }
+
+      case 5:
+        // Unmark all reference pictures.
+        dpb_.MarkAllUnusedForRef();
+        MaxLongTermFrameIdx_ = -1;
+        curr_pic_->mem_mgmt_5 = true;
+        break;
+
+      case 6: {
+        // Replace long term reference pictures with current picture.
+        // First unmark if any existing with this LongTermFrameIdx...
+        PicsVector long_terms;
+        dpb_.GetLongTermRefPicsAppending(long_terms);
+        for_each(long_terms.begin(), long_terms.end(),
+                 std::bind2nd(MarkLongTermPicUnusedForRefByLongTermFrameIdx(),
+                              ref_pic_marking->long_term_frame_idx));
+
+        // and mark the current one.
+        curr_pic_->ref = true;
+        curr_pic_->long_term = true;
+        curr_pic_->LongTermFrameIdx = ref_pic_marking->long_term_frame_idx;
+        break;
+      }
+
+      default:
+        // Would indicate a bug in parser.
+        NOTREACHED();
+    }
+  }
+
+  return true;
+}
+
+// This method ensures that DPB does not overflow, either by removing
+// reference pictures as specified in the stream, or using a sliding window
+// procedure to remove the oldest one.
+// It also performs marking and unmarking pictures as reference.
+// See spac 8.2.5.1.
+void VaapiH264Decoder::ReferencePictureMarking() {
+  if (curr_pic_->idr) {
+    // If current picture is an IDR, all reference pictures are unmarked.
+    dpb_.MarkAllUnusedForRef();
+
+    if (curr_pic_->long_term_reference_flag) {
+      curr_pic_->long_term = true;
+      curr_pic_->LongTermFrameIdx = 0;
+      MaxLongTermFrameIdx_ = 0;
+    } else {
+      curr_pic_->long_term = false;
+      MaxLongTermFrameIdx_ = -1;
+    }
+  } else {
+    if (!curr_pic_->adaptive_ref_pic_marking_mode_flag) {
+      // If non-IDR, and the stream does not indicate what we should do to
+      // ensure DPB doesn't overflow, discard oldest picture.
+      // See spec 8.2.5.3.
+      if (curr_pic_->field == H264Picture::FIELD_NONE) {
+        DCHECK_LE(dpb_.CountRefPics(),
+                std::max<int>(parser_.GetSPS(curr_sps_id_)->max_num_ref_frames,
+                              1));
+        if (dpb_.CountRefPics() ==
+              std::max<int>(parser_.GetSPS(curr_sps_id_)->max_num_ref_frames,
+                            1)) {
+          // Max number of reference pics reached,
+          // need to remove one of the short term ones.
+          // Find smallest FrameNumWrap short reference picture and mark
+          // it as unused.
+          H264Picture* to_unmark = dpb_.GetLowestFrameNumWrapRefPic();
+          if (to_unmark == NULL) {
+            DLOG(ERROR) << "Couldn't find a short ref picture to unmark";
+            return;
+          }
+          to_unmark->ref = false;
+        }
+      } else {
+        // Shouldn't get here.

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

NOTIMPLEMENTED is LOG(ERROR).  Can you replace some usages of it with DCHECKs
instead?

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

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?

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

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

+        NOTIMPLEMENTED() << "Interlaced video not supported.";
+      }
+    } else {
+      // Stream has instructions how to discard pictures from DPB and how
+      // to mark/unmark existing reference pictures. Do it.
+      // Spec 8.2.5.4.
+      if (curr_pic_->field == H264Picture::FIELD_NONE) {
+        HandleMemoryManagementOps();
+      } else {
+        // Shouldn't get here.
+        NOTIMPLEMENTED() << "Interlaced video not supported.";
+      }
+    }
+  }
+}
+
+bool VaapiH264Decoder::FinishPicture() {
+  DCHECK(curr_pic_.get());
+
+  // Finish processing previous picture.
+  // Start by storing previous reference picture data for later use,
+  // if picture being finished is a reference picture.
+  if (curr_pic_->ref) {
+    ReferencePictureMarking();
+    prev_ref_has_memmgmnt5_ = curr_pic_->mem_mgmt_5;
+    prev_ref_TopFieldOrderCnt_ = curr_pic_->TopFieldOrderCnt;
+    prev_ref_PicOrderCntMsb_ = curr_pic_->PicOrderCntMsb;
+    prev_ref_pic_order_cnt_lsb_ = curr_pic_->pic_order_cnt_lsb;
+    prev_ref_field_ = curr_pic_->field;
+  }
+
+  // Remove unused (for reference or later output) pictures from DPB.
+  dpb_.RemoveUnused();
+
+  DVLOG(4) << "Finishing picture, DPB entries: " << dpb_.size()
+           << " Num available dec surfaces: "
+           << num_available_decode_surfaces_;
+
+  if (dpb_.IsFull()) {
+    // DPB is full, we have to make space for the new picture.
+    // Get all pictures that haven't been outputted yet.
+    PicsVector not_outputted;
+    dpb_.GetNotOutputtedPicsSortedByPOCAppending(not_outputted);
+    PicsVector::iterator output_candidate = not_outputted.begin();
+
+    // Keep outputting pictures until we can either output the picture being
+    // finished and discard it (if it is not a reference picture), or until
+    // we can discard an older picture that was just waiting for output and
+    // is not a reference picture, thus making space for the current one.
+    while (dpb_.IsFull()) {
+      // Maybe outputted enough to output current picture.
+      if (!curr_pic_->ref && (output_candidate == not_outputted.end()
+            || curr_pic_->PicOrderCnt < (*output_candidate)->PicOrderCnt)) {
+        // curr_pic_ is not a reference picture and no preceding pictures are
+        // waiting for output in DPB, so it can be outputted and discarded
+        // without storing in DPB.
+        if (!OutputPic(curr_pic_.get()))
+          return false;
+        goto no_store;
+      }
+
+      // Couldn't output current picture, so try to output the lowest PoC
+      // from DPB.
+      if (output_candidate != not_outputted.end()) {
+        if (!OutputPic(*output_candidate))
+          return false;
+
+        // If outputted picture wasn't a reference picture, it can be removed.
+        if (!(*output_candidate)->ref)
+          dpb_.RemoveByPOC((*output_candidate)->PicOrderCnt);
+      } else {
+        // Couldn't output current pic and couldn't do anything
+        // with existing pictures in DPB, so we can't make space.
+        // This should not happen.
+        DLOG(ERROR) << "Could not free up space in DPB!";
+        return false;
+      }
+    }
+    ++output_candidate;
+  }
+
+  // Store current picture for later output and/or reference (ownership now
+  // with the DPB).
+  dpb_.StorePic(curr_pic_.release());
+
+no_store:
+  return true;
+}
+
+bool VaapiH264Decoder::ProcessSPS(int sps_id) {
+  int width, height;
+  H264SPS* sps = parser_.GetSPS(sps_id);
+  DCHECK(sps);
+
+  if (sps->frame_mbs_only_flag == 0) {
+    // Fields/interlaced video not supported.
+    DLOG(INFO) << "frame_mbs_only_flag != 1 not supported";
+    return false;
+  }
+
+  if (sps->gaps_in_frame_num_value_allowed_flag) {
+    DLOG(INFO) << "Gaps in frame numbers not supported";
+    return false;
+  }
+
+  curr_sps_id_ = sps->seq_parameter_set_id;
+
+  // Calculate picture height/width (spec 7.4.2.1.1, 7.4.3).
+  width = 16 * (sps->pic_width_in_mbs_minus1 + 1);
+  height = 16 * (2 - sps->frame_mbs_only_flag)
+         * (sps->pic_height_in_map_units_minus1 + 1);
+
+  if ((pic_width_ != -1 || pic_height_ != -1) &&
+      (width != pic_width_ || height != pic_height_)) {
+    DLOG(INFO) << "Picture size changed mid-stream";
+    return false;
+  }
+
+  pic_width_ = width;
+  pic_height_ = height;
+  DVLOG(1) << "New picture size: " << pic_width_ << "x" << pic_height_;
+
+  MaxPicOrderCntLsb_ = 1 << (sps->log2_max_pic_order_cnt_lsb_minus4 + 4);
+  MaxFrameNum_ = 1 << (sps->log2_max_frame_num_minus4 + 4);
+
+  return true;
+}
+
+bool VaapiH264Decoder::ProcessPPS(int pps_id) {
+  H264PPS* pps = parser_.GetPPS(pps_id);
+  DCHECK(pps);
+
+  curr_pps_id_ = pps->pic_parameter_set_id;
+
+  return true;
+}
+
+bool VaapiH264Decoder::FinishPrevFrameIfPresent() {
+  // If we already have a frame waiting to be decoded, decode it and finish.
+  if (curr_pic_ != NULL) {
+    if (!DecodePicture())
+      return false;
+    return FinishPicture();
+  }
+
+  return true;
+}
+
+bool VaapiH264Decoder::ProcessSlice(H264SliceHeader* slice_hdr) {
+  prev_frame_num_ = frame_num_;
+  frame_num_ = slice_hdr->frame_num;
+
+  if (prev_frame_num_ > 0 && prev_frame_num_ < frame_num_ - 1) {
+    DLOG(INFO) << "Gap in frame_num!";
+    return false;
+  }
+
+  if (slice_hdr->field_pic_flag == 0)
+    MaxPicNum_ = MaxFrameNum_;
+  else
+    MaxPicNum_ = 2 * MaxFrameNum_;
+
+  // TODO posciak: switch to new picture detection per 7.4.1.2.4.
+  if (curr_pic_ != NULL && slice_hdr->first_mb_in_slice != 0) {
+    // This is just some more slice data of the current picture, so
+    // just queue it and return.
+    QueueSlice(slice_hdr);
+    return true;
+  } else {
+    // A new frame, so first finish the previous one before processing it...
+    if (!FinishPrevFrameIfPresent())
+      return false;
+
+    // and then start a new one.
+    return StartNewFrame(slice_hdr);
+  }
+}
+
+#define SET_ERROR_AND_RETURN()            \
+  do {                                    \
+    DLOG(ERROR) << "Error during decode"; \
+    state_ = kError;                      \
+    return VaapiH264Decoder::kDecError;   \
+  } while (0)
+
+VaapiH264Decoder::DecResult VaapiH264Decoder::DecodeInitial(int32 input_id) {
+  // Decode enough to get required picture size (i.e. until we find an SPS),
+  // if we get any slice data, we are missing the beginning of the stream.
+  H264NALU nalu;
+  H264Parser::Result res;
+
+  DCHECK_NE(state_, kUninitialized);
+
+  curr_input_id_ = input_id;
+
+  while (1) {
+    // Get next NALU looking for SPS or IDR if after reset.
+    res = parser_.ParseNextNalu(&nalu);
+    if (res == H264Parser::kEOStream) {
+      DLOG(INFO) << "Could not find SPS before EOS";
+      return kNeedMoreStreamData;
+    } else if (res != H264Parser::kOk) {
+      SET_ERROR_AND_RETURN();
+    }
+
+    DVLOG(4) << " NALU found: " << (int)nalu.nal_unit_type;
+
+    switch (nalu.nal_unit_type) {
+      case kH264NaluSPS:
+        res = parser_.ParseSPS(&curr_sps_id_);
+        if (res != H264Parser::kOk)
+          SET_ERROR_AND_RETURN();
+
+        if (!ProcessSPS(curr_sps_id_))
+          SET_ERROR_AND_RETURN();
+
+        // Just got information about the video size from SPS, so we can
+        // now allocate surfaces and let the client now we are ready to
+        // accept output buffers and decode.
+        if (!CreateVASurfaces())
+          SET_ERROR_AND_RETURN();
+
+        state_ = kDecoding;
+        return kReadyToDecode;
+
+      case kH264NaluIDRSlice:
+        // If after reset, should be able to recover from an IDR.
+        if (state_ == kAfterReset) {
+          H264SliceHeader slice_hdr;
+
+          res = parser_.ParseSliceHeader(&slice_hdr, &nalu);
+          if (res != H264Parser::kOk)
+            SET_ERROR_AND_RETURN();
+
+          if (!ProcessSlice(&slice_hdr))
+            SET_ERROR_AND_RETURN();
+
+          state_ = kDecoding;
+          return kReadyToDecode;
+        } // else fallthrough
+      case kH264NaluNonIDRSlice:
+      case kH264NaluPPS:
+        // Non-IDR slices cannot be used as resume points, as we may not
+        // have all reference pictures that they may require.
+        // fallthrough
+      default:
+        // Skip everything unless it's PPS or an IDR slice (if after reset).
+        DVLOG(4) << "Skipping NALU";
+        break;
+    }
+  }
+}
+
+void VaapiH264Decoder::SetStream(uint8* ptr, size_t size) {
+  DCHECK(ptr);
+  DCHECK(size);
+
+  // Got new input stream data from the client.
+  DVLOG(4) << "New input stream chunk at " << (void*) ptr
+           << " size:  " << size;
+  parser_.SetStream(ptr, size);
+}
+
+VaapiH264Decoder::DecResult VaapiH264Decoder::DecodeOneFrame(int32 input_id) {
+  // Decode until one full frame is decoded or return it or until end
+  // of stream (end of input data is reached).
+  H264Parser::Result par_res;
+  H264NALU nalu;
+
+  curr_input_id_ = input_id;
+
+  if (state_ != kDecoding) {
+    DLOG(ERROR) << "Decoder not ready: error in stream or not initialized";
+    return kDecError;
+  } else if (num_available_decode_surfaces_ < 1) {
+    DVLOG(4) << "No output surfaces available";
+    return kNoOutputAvailable;
+  }
+
+  // All of the actions below might result in decoding a picture from
+  // previously parsed data, but we still have to handle/parse current input
+  // first.
+  // Note: this may drop some already decoded frames if there are errors
+  // further in the stream, but we are OK with that.
+  while (1) {
+    par_res = parser_.ParseNextNalu(&nalu);
+    if (par_res == H264Parser::kEOStream)
+      return kNeedMoreStreamData;
+    else if (par_res != H264Parser::kOk)
+      SET_ERROR_AND_RETURN();
+
+    DVLOG(4) << "NALU found: " << (int)nalu.nal_unit_type;
+
+    switch (nalu.nal_unit_type) {
+      case kH264NaluNonIDRSlice:
+      case kH264NaluIDRSlice: {
+        H264SliceHeader slice_hdr;
+
+        par_res = parser_.ParseSliceHeader(&slice_hdr, &nalu);
+        if (par_res != H264Parser::kOk)
+          SET_ERROR_AND_RETURN();
+
+        if (!ProcessSlice(&slice_hdr))
+          SET_ERROR_AND_RETURN();
+        break;
+      }
+
+      case kH264NaluSPS:
+        int sps_id;
+
+        if (!FinishPrevFrameIfPresent())
+          SET_ERROR_AND_RETURN();
+
+        par_res = parser_.ParseSPS(&sps_id);
+        if (par_res != H264Parser::kOk)
+          SET_ERROR_AND_RETURN();
+
+        if (!ProcessSPS(sps_id))
+          SET_ERROR_AND_RETURN();
+        break;
+
+      case kH264NaluPPS:
+        int pps_id;
+
+        if (!FinishPrevFrameIfPresent())
+          SET_ERROR_AND_RETURN();
+
+        par_res = parser_.ParsePPS(&pps_id);
+        if (par_res != H264Parser::kOk)
+          SET_ERROR_AND_RETURN();
+
+        if (!ProcessPPS(pps_id))
+          SET_ERROR_AND_RETURN();
+        break;
+
+      default:
+        // skip NALU
+        break;
+    }
+
+    // If the last action resulted in decoding a frame, possibly from older
+    // data, return. Otherwise keep reading the stream.
+    if (frame_decoded_) {
+      frame_decoded_ = false;
+      return kDecodedFrame;
+    }
+  }
+}
+
+// static
+int VaapiH264Decoder::GetRequiredNumOfPictures() {
+  return kNumReqPictures;
+}
+
+base::LazyInstance<GLXFBConfig> VaapiH264Decoder::DecodeSurface::fb_config_ =
+    LAZY_INSTANCE_INITIALIZER;
+Display* VaapiH264Decoder::DecodeSurface::x_display_;
+