[Bug 781537] NVDEC - Nvidia Decoder plugin

[GStreamer (GNOME Bugzilla)]

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

--- Comment #43 from Per-Erik Brodin <per-erik.brodin at ericsson.com> ---
(In reply to Sebastian Dröge (slomo) from comment #41)

Thank you for the quick review!

> Review of attachment 352853 [details] [review]:
> 
> Just some initial comments below
> 
> ::: configure.ac
> @@ +1920,3 @@
> +  save_CPPFLAGS="$CPPFLAGS"
> +  CPPFLAGS="$NVCUVID_CFLAGS $save_CPPFLAGS"
> +  AC_CHECK_HEADER([nvcuvid.h], [HAVE_NVCUVID_H=yes],
> 
> Why don't you use pkg-config?

At least on Debian/Ubuntu there is no .pc file provided by nvidia-cuda-dev.
nvcuvid.h is located directly under /usr/include so typically you don’t have to
set the NVCUVID_CFLAGS.

> 
> ::: sys/nvdec/gstnvdec.c
> @@ +120,3 @@
> +  CUgraphicsResource *resources;
> +  guint num_resources;
> +} GstNvDecCudaGraphicsResourcesMeta;
> 
> Why is this a GstMeta instead of a custom GstMemory (e.g. based on
> GstGLBaseMemory)?

I considered that but then we would also need a custom allocator and maybe a
custom buffer pool and then the downstream element won’t be able to do the
allocation. I think using a GstMeta is the best temporary solution until we
have a GstCUDA library.

> 
> @@ +617,3 @@
> +  else if (!g_strcmp0 (caps_name, "video/x-h265"))
> +    parser_params.CodecType = cudaVideoCodec_HEVC;
> +  else {
> 
> Add some {} above here

Done

> 
> @@ +735,3 @@
> +    pending_frame = pending_frames->data;
> +    if (!pending_frame->system_frame_number)
> +      break;
> 
> Why?

Added a comment to clarify that we are looking for the oldest unfinished frame
that has not yet been passed to the decoder.

> 
> @@ +790,3 @@
> +        decode_params = (CUVIDPICPARAMS *) item->data;
> +        pending_frame = pending_frames->data;
> +        pending_frame->system_frame_number = decode_params->CurrPicIdx + 1;
> 
> You should never override the system_frame_number, it's read-only from a
> subclass point of view

Ok, fixed. I thought the purpose of the separate decode_frame_number was so
that system_frame_number could be changed. At least currently there is nothing
in the base class that breaks when changing it.

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