VAVDA: Redesign stage 1.

content/common/gpu/media/vaapi_delegate.h

+// Copyright (c) 2013 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 VaapiDelegate, used by
+// VaapiVideoDecodeAccelerator and VaapiH264Decoder to interface
+// with libva (VA-API library for hardware video decode).
+
+#ifndef CONTENT_COMMON_GPU_MEDIA_VAAPI_DELEGATE_H_
+#define CONTENT_COMMON_GPU_MEDIA_VAAPI_DELEGATE_H_
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "media/base/video_decoder_config.h"
+#include "third_party/libva/va/va.h"
+#include "third_party/libva/va/va_x11.h"
+
+namespace content {
+
+// A VA-API-specific decode surface used by VaapiH264Decoder to decode into

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Since this class isn't used in this .h (or .cc file, other than definition) ISTM
it should either move out of this header or at least move below VaapiDelegate,
which is what this header is named after.

[Pawel Osciak, 2013/05/21 22:32:35]

Didn't want to create a separate header for it, I'll move below.

[Ami GONE FROM CHROMIUM, 2013/05/22 23:59:47]

I think this is a bad tradeoff.  It's a separate thing with separate needs and
is unused by the class this file is named after so IMO it should have its own
header.

[Pawel Osciak, 2013/05/24 01:46:39]

Done.

+// and use as reference for decoding other surfaces. It is also handed by the
+// decoder to VaapiVideoDecodeAccelerator when the contents of the surface are
+// ready and should be displayed. VAVDA converts the surface contents into an
+// X Pixmap bound to a texture for display and releases its reference to it.
+// Decoder releases its references to the surface when it's done decoding and
+// using it as reference. Note that a surface may still be used for reference
+// after it's been sent to output and also after it is no longer used by VAVDA.
+// Thus, the surface can be in use by both VAVDA and the Decoder at the same
+// time, or by either of them, with the restriction that VAVDA will never get
+// the surface until the contents are ready, and it is guaranteed that the
+// contents will not change after that.
+// When both the decoder and VAVDA release their references to the surface,
+// it is freed and the release callback is executed to put the surface back
+// into available surfaces pool, which is managed externally.
+class VASurface : public base::RefCountedThreadSafe<VASurface> {
+ public:
+  // Provided by user, will be called when all references to the surface
+  // are released.
+  typedef base::Callback<void(VASurfaceID)> ReleaseCB;
+
+  VASurface(VASurfaceID va_surface_id, const ReleaseCB& release_cb);
+
+  VASurfaceID id() {
+    return va_surface_id_;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<VASurface>;
+  ~VASurface();
+
+  VASurfaceID va_surface_id_;

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

const, if you like

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+  ReleaseCB release_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(VASurface);
+};
+
+// This class handles VA-API calls and ensures proper locking of VA-API calls
+// to libva, the userspace shim to the HW decoder driver. libva is not
+// thread-safe, so we have to perform locking ourselves.

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Does this mean the public methods of this class may be called on any thread?  If
so, doco.
For example, what are the semantics of one thread calling SubmitBuffer and
another calling DestroyPendingBuffers "at the same time"?  

[Pawel Osciak, 2013/05/21 22:32:35]

It's safe to call from any thread, things will not explode. But some obvious
sanity is required for things to work; even if nothing will explode if the above
situation happens, if one thread blows up buffers for another, the decode will
fail and return an error. But no leaks or use after free will happen, as
Decode() operations are atomic, both in this class and in the driver.

+//
+// This class is responsible for managing VAAPI connection, contexts and state.
+// It is also responsible for managing and freeing VABuffers (not VASurfaces),
+// which are used to queue decode parameters and slice data to the HW decoder,
+// as well as underlying memory for VASurfaces themselves.

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Does this translate to "none of the other files in this directory should need to
include anything from third_party/libva directly"?

[Pawel Osciak, 2013/05/21 22:32:35]

Correct.

+class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> {

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Is this actually a VaapiWrapper?

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Why refcount?
(in particular, can VAVDA hold a scoped_ptr and the decoder hold a raw pointer
and things still be safe?)

[Pawel Osciak, 2013/05/21 22:32:35]

Currently yes, as the decoder doesn't do anything with the delegate in its
destructor. I don't expect it to start doing that, but if it were to, we would
have to ensure the decoder was destroyed before the delegate. So is what you
describe preferred? This was just defensive programming, what's the usual thing
we do in this situation?

[Pawel Osciak, 2013/05/21 22:32:35]

For me delegate and wrapper are almost synonymous in this case, but I can call
it a wrapper if you prefer. Do you?

[Ami GONE FROM CHROMIUM, 2013/05/22 23:59:47]

Refcounting abdicates responsibility for ownership management.
It is "more defensive" than managing ownership explicitly, but only insofar as
leaking all allocations is "more defensive" than cleaning up unused garbage ;)

If refcounting is not needed please manage ownership explicitly (via
scoped_ptr).

[Pawel Osciak, 2013/05/24 01:46:39]

Done.

+ public:
+  // |report_error_cb| will be used to report errors for UMA purposes, not
+  // to report errors to clients.

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Why not rename it to something clearer, then?  
report_error_to_uma_cb ?

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+  static scoped_refptr<VaapiDelegate> Create(
+      media::VideoCodecProfile profile,
+      Display* x_display,
+      const base::Closure& report_error_cb);
+
+  // Create |num_surfaces| backing surfaces in driver for VASurfaces, each
+  // of size |width|x|height|, and return their IDs to be managed and later
+  // wrapped in VASurfaces.

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Returns success.

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+  bool CreateSurfaces(int width, int height,

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

here and elsewhere, better to use a gfx::Size than a pair of ints (whose order
is easily confused)

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

The impl of this assumes that this method is called no more than once during the
lifetime of a given VaapiDelegate object.  The doco should say so.

(this will break when you teach the VDAs to deal with midstream resize because
you'll want to allow the VDA impls to accept multiple AssignPictureBuffer calls
during their lifetime)

[Pawel Osciak, 2013/05/21 22:32:35]

Done, here and in other places.

[Pawel Osciak, 2013/05/21 22:32:35]

Not exactly. CreateSurfaces() -> DestroySurfaces() -> CreateSurfaces() is a
perfectly valid sequence.
Well, that's why DestroySurfaces() is not private. Doing DestroySurfaces() from
Create - if someone called it again - is not safe either, because clients may
still be using them.  So I only have a DCHECK in Create for this.

I could have them refcounted, but chose not to. First of all, I'd need a second
refcounted wrapper around IDs, and I have one already (VASurface, but it's not
for destroying the IDs, just for putting VASurfaces back into a freelist), which
feels a bit like overdoing this. And second of all, each such surface would also
have to keep a ref to the delegate. So it's a bit meh...

Are you ok with just the DCHECK and a contract in documentation?

+                      size_t num_surfaces,
+                      std::vector<VASurfaceID>& va_surfaces);

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Non non-const references.  
Here and elsewhere, which includes at least:
+                               VAProfile& va_profile) {
+                                   std::vector<VASurfaceID>& va_surfaces) {
+                      std::vector<VASurfaceID>& va_surfaces);

according to a quickie grep.

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+
+  // Free all memory allocated in CreateSurfaces.
+  void DestroySurfaces();

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Should be private.

[Pawel Osciak, 2013/05/21 22:32:35]

See above (l.78).

+
+  // Submit parameters or slice data of |va_buffer_type|, copying them from
+  // |buffer| of size |size|, into HW decoder. The data in |buffer| is no
+  // longer needed and can be freed after this method returns.
+  // Data submitted via this method awaits in the HW decoder until
+  // DecodeAndDestroyPendingBuffers is called to execute or
+  // DestroyPendingBuffers is used to cancel a pending decode.
+  bool SubmitBuffer(VABufferType va_buffer_type, size_t size, void* buffer);
+
+  // Cancel and destroy all buffers queued to the HW decoder via SubmitBuffer.
+  // Useful when a pending decode is to be cancelled (on reset or error).
+  void DestroyPendingBuffers();
+
+  // Execute decode in hardware and destroy pending buffers.
+  bool DecodeAndDestroyPendingBuffers(VASurfaceID va_surface_id);

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Why is "Destroy" part of the name of this function?
(IOW, what client-visible state is being destroyed)

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

What does the bool return value mean?
I can imagine at least:
- successfully decoded a pic; more data is available to be decoded
- successfully decoded a pic; no more data is available to be decoded
- failed to decode a pic b/c of error
- failed to decode a pic b/c of insufficient data being submitted
...

[Pawel Osciak, 2013/05/21 22:32:35]

I felt documentation for SubmitBuffer explained it well enough:

// Data submitted via this method awaits in the HW decoder until
// DecodeAndDestroyPendingBuffers is called to execute or
// DestroyPendingBuffers is used to cancel a pending decode

Destroy means "dispose of the buffers I submitted to the decoder via
SubmitBuffer".

[Pawel Osciak, 2013/05/21 22:32:35]

Decode() is for exactly one picture in vaapi. So the caller must've submitted
correct and sufficient data to decode exactly one picture. Otherwise the decode
will fail, and it's not really intended to be recoverable right now. So 1) and
4) cannot happen.

I could've elaborated on this, but let's face it, this is not a reusable class
that anybody can just start using. The client is tightly tied to this class (or
perhaps the other way around) and has to know exactly how to use vaapi in all
the other places, where it constructs VABuffers, submits them, etc. So I didn't
want to overdocument, it's really not a very "public" interface that just about
anyone could start using.

+
+
+  // Put data from |va_surface_id| into x_pixmap of size |width|x|height|,

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

|x_pixmap|

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+  // converting/scaling to appropriate size.
+  bool PutSurfaceIntoPixmap(VASurfaceID va_surface_id,

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Does this require making a context current (or somesuch) before calling this?

[Pawel Osciak, 2013/05/21 22:32:35]

No, thankfully. It's not a GL call.

+                            Pixmap x_pixmap,
+                            int width, int height);

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

dest_width & dest_height ?

[Pawel Osciak, 2013/05/21 22:32:35]

dest_size :)

+
+  // Do any necessary initialization before the sandbox is enabled.
+  static void PreSandboxInitialization();
+
+ private:
+  friend class base::RefCountedThreadSafe<VaapiDelegate>;
+  VaapiDelegate();
+  ~VaapiDelegate();
+
+  bool Initialize(media::VideoCodecProfile profile,

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Should be private, and can be inlined into Create().

[Pawel Osciak, 2013/05/21 22:32:35]

It is private :) I didn't want to inline, because I'd have to have a '= NULL'
and return every time something failed. This way I have them once.

+                  Display* x_display,
+                  const base::Closure& report_error_cb);
+  void Deinitialize();

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

inline into dtor, since it's the only callsite and also because it's not
possible to re-Initialize() after Deinitialize() runs?

[Pawel Osciak, 2013/05/21 22:32:35]

I'd prefer to keep it this way, makes the destructor cleaner and code more
understandable with Initialize-Deinitialize pair. I really wanted to keep it
tidy and clear. And yes, it's ok to re-Initialize() after Deinitialize(), even
if nothing does that now.

+
+  bool SubmitDecode(VASurfaceID va_surface_id);

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

doco methods whose purpose is unclear.  For example here, it's unclear why this
is not inlined into DecodeAndDestroyPendingBuffers.

Also, it's extra unclear because this method is named SubmitDecode, but is not
called by SubmitBuffer.

[Pawel Osciak, 2013/05/21 22:32:35]

To be honest I don't feel that way:

SubmitBuffer();
SubmitBuffer();
SubmitBuffer();
...
SubmitDecode();

feels like a natural sequence to me. I don't see why SubmitDecode should be
called by SubmitBuffer...

But since you thought of it like that, it's my failure, not documenting it well
enough...

+
+  // Libva is not thread safe, so we have to do locking for it ourselves.
+  // This lock is to be taken for the duration of all VA-API calls and for

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

I think the only API that makes sense is that every public method of this class
acquires the lock for the duration of its execution.

A salient point for why this strategy _might_ be correct is that this class is
fully synchronous.  I think it's worth calling out in the class doco that
explains about threading.

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+  // the entire decode execution sequence in DecodeAndDestroyPendingBuffers().

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

...and yet, it is not (see comment there).

[Pawel Osciak, 2013/05/21 22:32:35]

See comment there please. Destroy() doesn't have to be in the same critical
section as Decode, just cannot come in the middle of it, which it doesn't.

+  base::Lock va_lock_;

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

naming this va_lock_ implies it should be held only around libva calls, which is
incorrect (you want to hold it across libva calls within the same VaapiDelegate
function)

[Pawel Osciak, 2013/05/21 22:32:35]

This is a lock around each libva call or calls that need to be called atomically
in a sequence. There is only one example of the latter, in Decode().

+
+  // Allocated backing memory ids for VASurfaces.

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

This comment emphasizes "allocated" in a surprising way to me.

[Pawel Osciak, 2013/05/21 22:32:35]

Well, this was also kind-of documenting what the ids were. Anyway, removing.

+  scoped_ptr<VASurfaceID[]> va_surface_ids_;
+  size_t num_va_surfaces_;

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

why not std::vector<VASurfaceID> instead and avoid the need for the explicit
num_ ?

If the motivation here was that you need a contiguous buffer to feed to VAAPI_
functions, you can always do:
va_surface_ids_.resize(num_surfaces_or_whatever);
VAAPI_Foo(&va_surface_ids_[0], va_surface_ids_.size());

[Pawel Osciak, 2013/05/21 22:32:35]

Yeah, I know this full well, and I even thought about it, but maybe it's the C
programmer in me that I have this weird aversion and mistrust to ever doing
this...
Maybe it's a good time to get over it. Done.

+
+  // VA handles.
+  VADisplay va_display_;
+  VAConfigID va_config_id_;
+  VAContextID va_context_id_;

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Would be useful to annotate these with when in the lifecycle they become
valid/invalid.

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+
+  // Data queued up for HW decoder, to be committed on next HW decode.
+  std::vector<VABufferID> pending_slice_bufs_;
+  std::vector<VABufferID> pending_va_bufs_;

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

Do these really need to be separate?
(AFAICT they're all fed to the same APIs; does VAAPI require to see non-slices
before slices??)

[Pawel Osciak, 2013/05/21 22:32:35]

All the code that used VAAPI that I've seen, including the tests and code
written by Intel VAAPI developers does the same thing. Even if it was possible
now, who knows when this might break. I'd prefer not to go against the flow
here, as much as I don't like this separation myself either.

[Ami GONE FROM CHROMIUM, 2013/05/22 23:59:47]

You just gave a great definition of what it means to cargo-cult.

[Pawel Osciak, 2013/05/24 01:46:39]

Not exactly. I'm saying that even if it was ok to do, it's sadly irrelevant, if
everybody else does it this way, if libva/vaapi at some point develops a related
bug for this/starts depending on it/stops working otherwise, we will be the only
ones that will start exploding for seemingly inexplicable reasons and will most
probably waste a mountain of time debugging, staring at raw command buffers sent
to the GPU for weeks. Trust me, it wouldn't be the first time.

+
+  // Called to report decoding error to UMA, not used to indicate errors
+  // to clients.
+  base::Closure report_error_cb_;
+
+  // Lazily initialize static data after sandbox is enabled.  Return false on
+  // init failure.
+  static bool PostSandboxInitialization();

[Ami GONE FROM CHROMIUM, 2013/05/17 23:19:15]

methods go above members

[Pawel Osciak, 2013/05/21 22:32:35]

Done.

+
+  // Has static initialization of pre-sandbox components completed successfully?
+  static bool pre_sandbox_init_done_;
+
+  DISALLOW_COPY_AND_ASSIGN(VaapiDelegate);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_COMMON_GPU_MEDIA_VAAPI_DELEGATE_H_