Created attachment 340068 [details] [review]
[PATCH gst-plugins-bad] msdk: Add H.264 decoder

Awesome \o/

Created attachment 340072 [details] [review]
[PATCH gst-plugins-bad v2] msdk: Add H.264 decoder

Changes since v1:
- Update copyright in *h264dec.*

Created attachment 340555 [details] [review]
[PATCH gst-plugins-bad v3] msdk: Add H.264 decoder

Changes since v2:
- clang warns (incorrectly, I think) on the braces in the
  initializer expression for mfxBitstream. Change to a memset to
  quiet that.

Created attachment 341510 [details] [review]
[PATCH gst-plugins-bad v4] msdk: Add H.264 decoder

Changes since v3:
- Fixes to decoding with hardware=false, specifically (1)MemId
  needs to be NULL for software decoding with system memory,
  (2)Surfaces may be unlocked by the msdk without being outputted
  from DecodeFrameAsync, so handle unreffing/unmapping properly,
  (3)Surfaces may still be locked for reference when outputted, so
  map them as READWRITE so downstream elements can still read
  them.

Review of attachment 341510 [details] [review]:

::: sys/msdk/gstmsdkdec.c
@@ +163,3 @@
+}
+
+static GstFlowReturn gst_msdkdec_drain (GstVideoDecoder * decoder);

Please reorder the code to avoid this forward declaration.

@@ +166,3 @@
+
+static void
+gst_msdkdec_set_property (GObject * object, guint prop_id, const GValue * value,

My preference and to be more consistent with the encoder base class is to have gobject overrides, _set_property, _get_property and _finalize, at the bottom just before the class_init.

::: sys/msdk/gstmsdkdec.h
@@ +81,3 @@
+  GstVideoDecoderClass parent_class;
+
+    gboolean (*set_format) (GstMsdkDec * decoder);

This vmethod is not used, maybe just drop it.

It is customary in GStreamer to have the init/class_init functions fairly early in the file and then have lots of forward declarations for the various vfuncs, for what it's worth. But that can be done either way of course :)

Created attachment 341839 [details] [review]
[PATCH gst-plugins-bad v5] msdk: Add H.264 decoder

Changes since v4:
- removed the unused set_format vfunc, reordered some functions,
  whitespace adjustments.

Created attachment 341840 [details] [review]
[PATCH gst-plugins-bad v5] msdk: Add H.264 decoder

attachment 341839 [details] [review] was a mis-upload (commit message of v5, body of v4). Fixed here.

Review of attachment 341840 [details] [review]:

Looks good to me.

Comment on attachment 341840 [details] [review]
[PATCH gst-plugins-bad v5] msdk: Add H.264 decoder

commit 83774c3eb9662bedc4dddcbea3fd17d237d25416
Author: Scott D Phillips <scott.d.phillips@intel.com>
Date:   Mon Nov 7 12:32:38 2016 -0800

    msdk: Add H.264 decoder
    
    The decoder only supports system memory output presently.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=774587