[Bug 781537] NVDEC - Nvidia Decoder plugin

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=781537

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #352564|none                        |needs-work
             status|                            |

--- Comment #31 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 352564:
 --> (https://bugzilla.gnome.org/review?bug=781537&attachment=352564)

Sorry for the late review! Some comments below

::: configure.ac
@@ +2036,3 @@
+    PKG_CHECK_MODULES([CUDA], [cuda-7.5 cudart-7.5],, [
+      PKG_CHECK_MODULES([CUDA], [cuda-7.0 cudart-7.0],, [
+        PKG_CHECK_MODULES([CUDA], [cuda-6.5 cudart-6.5],, [

Maybe make this a for loop over all the versions, also check for unversioned
(and then check the module's version number instead). And 8.0

::: sys/nvdec/gstnvh264dec.c
@@ +206,3 @@
+      GST_DEBUG_FUNCPTR (gst_nvh264dec_set_format);
+  video_decoder_class->drain = GST_DEBUG_FUNCPTR (gst_nvh264dec_drain);
+  video_decoder_class->reset = GST_DEBUG_FUNCPTR (gst_nvh264dec_reset);

reset() is deprecated, finish() / flush() / drain() replace it

@@ +224,3 @@
+{
+  GstNvh264decPrivate *pnvh264dec = GST_NVH264DEC_GET_PRIVATE (nvh264dec);
+  IS_CUDA_CALL_SUCCSESS (nvh264dec, cuInit (0));

Can this be done multiple times?

@@ +233,3 @@
+  pnvh264dec->host_data_size = 0;
+  pnvh264dec->width = 0;
+  pnvh264dec->height = 0;

Usually you would just store the output GstVideoCodecState and use the
width/height from the GstVideoInfo in there

@@ +234,3 @@
+  pnvh264dec->width = 0;
+  pnvh264dec->height = 0;
+  g_mutex_init (&pnvh264dec->queue_mutex);

Need to clear() mutex (and probably other things) in GObject::finalize()

@@ +317,3 @@
+  pnvh264dec->is_frame_in_use[cuviddisp->picture_index] = TRUE;
+  // Wait until we have a free entry in the display queue (should never block
if we have enough
+  // entries)

Instead of a loop without waiting here, maybe use a GCond? Also maybe these
queues should be of configurable size? You could use a GstQueueArray

@@ +623,3 @@
+  if (gst_structure_get_int (pad_struct, "width", &width)) {
+  }
+  if (gst_structure_get_int (pad_struct, "height", &height)) {

These should already be in the GstVideoInfo in state

@@ +626,3 @@
+  }
+
+  codec_data = gst_structure_get_string (pad_struct, "codec_data");

It's never a string but a GstBuffer, and will only exist for stream-format=avc

@@ +753,3 @@
+  }
+
+  memcpy (omap_info.data, pnvh264dec->host_data, pnvh264dec->host_data_size);

Why do you memcpy twice here? Once with cuMemcpyDtoH(), once with memcpy()

Ideally (in a later version) we would somehow implement zerocopy here if
downstream can support it

@@ +822,3 @@
+      && gst_nvh264dec_dequeue_frame (pnvh264dec, &cuviddisp)) {
+    GstFlowReturn ret =
+        gst_nvh264dec_send_decoded_frame (decoder, &cuviddisp, frame);

This seems to assume that coding and display order are the same (which is not
the case), and that the frame that is currently output is the same as the input
frame (which is also not always the case). You need to get the correct
GstVideoCodecFrame here that corresponds to the input frame that we're going to
output now.

There's various API in GstVideoDecoder for tracking these.

::: sys/nvdec/gstnvh264plugin.c
@@ +36,3 @@
+GST_PLUGIN_DEFINE (GST_VERSION_MAJOR,
+    GST_VERSION_MINOR,
+    nvh264dec,

Has to be nvdec as that's the plugin filename

@@ +39,3 @@
+    "Nvidia cuda decoder plugin",
+    plugin_init,
+    VERSION, "BSD", "Setplex GStreamer plugins", "http://www.setplex.com")

VERSION, GST_LICENSE, GST_PACKAGE_NAME, GST_PACKAGE_ORIGIN

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.