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

content/common/gpu/media/vaapi_video_decode_accelerator.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// This file contains an implementation of VideoDecoderAccelerator
+// that utilizes hardware video decoder present on Intel CPUs.
+
+#ifndef CONTENT_COMMON_GPU_MEDIA_VAAPI_VIDEO_DECODE_ACCELERATOR_H_
+#define CONTENT_COMMON_GPU_MEDIA_VAAPI_VIDEO_DECODE_ACCELERATOR_H_
+
+#include "vaapi_h264_decoder.h"
+
+#include <map>

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

Unused.
(please make a pass on the CL to verify headers are only included where
necessary)

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+#include <queue>
+#include <utility>
+#include <vector>
+
+#include <GL/glx.h>
+
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop.h"
+#include "base/shared_memory.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/threading/thread.h"
+#include "media/base/bitstream_buffer.h"
+#include "media/video/picture.h"
+#include "media/video/video_decode_accelerator.h"
+
+// Class to provide video decode acceleration for Intel systems with hardware
+// support for it, and on which libva is available.
+// Decoding tasks are performed in a separate decoding thread.
+class VaapiVideoDecodeAccelerator
+  : public media::VideoDecodeAccelerator,
+    NON_EXPORTED_BASE(public base::NonThreadSafe) {

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

I don't think you understand what NonThreadSafe is for :)
(you never call CalledOnValidThread(), and this class actually *does* expect to
get called on multiple threads).
Drop this base.

[Pawel Osciak, 2012/05/03 16:22:07]

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

+ public:
+
+  VaapiVideoDecodeAccelerator(media::VideoDecodeAccelerator::Client* client);

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

s/media::VideoDecodeAccelerator::// I think (since this is already a VDA).  
This also applies elsewhere in this file and in the .cc file.

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+
+  // media::VideoDecodeAccelerator implementation.
+  bool Initialize(media::VideoCodecProfile profile) OVERRIDE;

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

missing "virtual" keyword from many methods.

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+  void Decode(const media::BitstreamBuffer& bitstream_buffer) OVERRIDE;
+  virtual void AssignPictureBuffers(
+      const std::vector<media::PictureBuffer>& buffers) OVERRIDE;
+  void ReusePictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  void Flush() OVERRIDE;
+  void Reset() OVERRIDE;
+  void Destroy() OVERRIDE;
+
+  // Used by user of this class to pass X/GLX state.
+  void SetGlxState(Display* x_display, GLXContext glx_context);
+
+ private:
+  virtual ~VaapiVideoDecodeAccelerator();
+
+  // Notify the client that initialization is finished (successful or not).

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

See my comment in the .cc file, but I think you can drop all these methods
(through StopOnError) that just wrap client_ calls by binding to the client call
in the first place.

[Pawel Osciak, 2012/05/03 16:22:07]

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

+  void NotifyInitializeDone();
+
+  // Notify the client that the input buffer has been consumed.
+  void NotifyInputBufferRead(int input_buffer_id);
+
+  // Ensure data has been synced with the output texture and notify
+  // the client it is ready for displaying.
+  void SyncAndNotifyPictureReady(int32 input_id, int32 output_id);
+
+  // Notify the client that Reset or Flush have finished.
+  void NotifyResetDone();
+  void NotifyFlushDone();
+
+  // Ask the client to provide |num_pics| textures of size |width| per |height|
+  void RequestPictureBuffers(int num_pics, int width, int height);
+
+  // Notify the client that an error has occured and decoding cannot continue.
+  void StopOnError(media::VideoDecodeAccelerator::Error error);
+
+  // Map the received input buffer into this process' address space and
+  // queue it for use.

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

s/use/decode/

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+  void MapAndQueueNewInputBuffer(
+      const media::BitstreamBuffer& bitstream_buffer);
+
+  // Request and sets up a new input stream buffer for decoder if available.
+  // Will release and give back the current input buffer, if present.
+  void TryGetNewInputBuffer();
+
+  // Decoding task, executed in decoder's thread context.
+  void DecodeTask();
+
+  // Give back an output buffer to the decoder after displaying it.
+  // Must be executed on the decoder thread.
+  void ReusePictureTask(int32 picture_buffer_id);
+
+  // Force the decoder to flush all output pictures.
+  // Must be run on the decoder thread.
+  void FlushTask();
+
+  // Put the decoder into reset state.
+  // Must be run on the decoder thread.
+  void ResetTask();
+
+  // Used by the decoder thread to request more input stream.

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

s/stream/streams/

[Pawel Osciak, 2012/05/03 16:22:07]

I chose "more of the input stream" instead.

+  // Must be run on the decoder thread and will sleep until new input
+  // becomes available.
+  void WaitForInput();
+
+  // Client-provided X/GLX state.
+  Display* x_display_;
+  GLXContext glx_context_;
+
+  // An input buffer awaiting consumption, provided by the client.
+  struct InputBuffer {
+    int32 id;
+    size_t size;
+    scoped_ptr<base::SharedMemory> shm;
+  };
+
+  // Queue for incoming input buffers.
+  typedef std::queue<InputBuffer*> InputBuffers;
+  InputBuffers input_buffers_;
+  // Current input buffer set in decoder.

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

s/set in/at/

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+  scoped_ptr<InputBuffer> curr_input_buffer_;
+
+  // Signalled if a valid stream is set up in decoder. Used to synchronize
+  // between decoder and main thread to safely update current stream in decoder
+  // from the |input_buffers_| queue.
+  base::WaitableEvent input_ready_;
+

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

In your quest to keep threading concerns out of the decoder, you have the result
that both VAVDA & the decoder have data members that are accessed on exactly one
thread, and some that are accessed on both, but none of this is documented. 
While the decoder gains the benefit of theoretically being runnable in an
unthreaded environment without cognitive dissonance in the comments, it pays in
being very difficult to verify that no races exist.  IMO this is very
unfortunate.

[Pawel Osciak, 2012/05/03 16:22:07]

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

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

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

Both cases are documented in .cc.

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

+  // Main thread's message loop
+  MessageLoop* message_loop_;
+
+  // To expose client callbacks from VideoDecodeAccelerator.
+  // NOTE: all calls to this object *MUST* be executed in message_loop_.
+  Client* client_;
+
+  // True if output pictures have been requested from the client.
+  bool pictures_requested_;
+
+  // True if we are after reset and need to resume.
+  bool after_reset_;
+
+  // True if we are resetting.
+  bool resetting_;
+
+  base::Thread decoder_thread_;
+  content::VaapiH264Decoder decoder_;
+
+  // Callback passed to the decoder, which it will use to signal readiness
+  // of an output picture to be displayed.
+  static void OutputPicCallback(VaapiVideoDecodeAccelerator* vavda,

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

It's silly to use a static method and pass |this| as its first param.
Just drop "static" and the first param and bind to (not Unretained!) this.

[Pawel Osciak, 2012/05/03 16:22:07]

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

+                                int32 input_id,
+                                int32 output_id);
+
+  DISALLOW_COPY_AND_ASSIGN(VaapiVideoDecodeAccelerator);
+};
+
+#endif  // CONTENT_COMMON_GPU_MEDIA_VAAPI_VIDEO_DECODE_ACCELERATOR_H_
+