[Bug 725145] libde265 based HEVC/H.265 decoder plugin

Thu Aug 28 01:29:47 PDT 2014

https://bugzilla.gnome.org/show_bug.cgi?id=725145
  GStreamer | gst-plugins-bad | git

Sebastian Dröge (slomo) <slomo> changed:

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

--- Comment #7 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-08-28 08:29:44 UTC ---
Review of attachment 284591:
 --> (https://bugzilla.gnome.org/review?bug=725145&attachment=284591)

The file naming could be a bit closer to what we have elsewhere :)
gstde265dec.c / .h and plugin.c for the one with the plugin_init

::: ext/libde265/Makefile.am
@@ +7,3 @@
+libgstlibde265_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(GST_BASE_CFLAGS)
$(GST_CFLAGS) \
+    $(LIBDE265_CFLAGS)
+libgstlibde265_la_LIBADD = $(GST_PLUGINS_BASE_LIBS) $(GST_BASE_LIBS)
$(GST_LIBS) -lgstvideo-$(GST_API_VERSION) \

-lgstvideo should be between PLUGINS_BASE_LIBS and BASE_LIBS

::: ext/libde265/libde265-dec.c
@@ +47,3 @@
+
+#if !defined(LIBDE265_NUMERIC_VERSION) || LIBDE265_NUMERIC_VERSION <
0x00080000
+#error "You need libde265 0.8 or newer to compile this plugin."

The pkg-config check should've caught this already

@@ +51,3 @@
+
+// use two decoder threads if no information about
+// available CPU cores can be retrieved

No C99 comments

@@ +60,3 @@
+    GST_PAD_SINK,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("video/x-h265")

Add information about the supported stream-formats, profiles, tiers, levels,
etc here

@@ +66,3 @@
+    GST_PAD_SRC,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE ("{ I420 }"))

You can omit the {} here

@@ +133,3 @@
+      g_param_spec_enum ("mode", "Input mode",
+          "Input mode of data to decode", GST_TYPE_LIBDE265_DEC_MODE,
+          DEFAULT_MODE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

This should be negotiated via caps. The stream-format field

@@ +138,3 @@
+      gst_param_spec_fraction ("framerate", "Frame Rate",
+          "Frame rate of images in raw stream", 0, 1, 100, 1, DEFAULT_FPS_N,
+          DEFAULT_FPS_D, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

THis should also be negotiated via caps

@@ +224,3 @@
+      dec->fps_n = gst_value_get_fraction_numerator (value);
+      dec->fps_d = gst_value_get_fraction_denominator (value);
+      GST_DEBUG ("Framerate set to %d/%d", dec->fps_n, dec->fps_d);

Use GST_DEBUG_OBJECT() and related macros whenever inside a element
implementation

@@ +255,3 @@
+  GstVideoFrame vframe;
+  GstBuffer *buffer;
+  int mapped;

gboolean

@@ +266,3 @@
+  gst_video_codec_frame_unref (ref->frame);
+  gst_buffer_replace (&ref->buffer, NULL);
+  free (ref);

Use g_malloc() and g_free() or g_slice_new() and g_slice_free()

@@ +283,3 @@
+  GstVideoInfo *info;
+
+  frame = gst_video_decoder_get_frame (base, dec->frame_number);

Why this usage of dec->frame_number, which is bsaically the current system
frame number?

@@ +404,3 @@
+#else
+#warning "Don't know how to get number of CPU cores, will use the default
thread count"
+  threads = DEFAULT_THREAD_COUNT;

Make it a property. 0 would mean automatic (i.e. get it from sysconf), every
other value would be used directly. You don't always wants to use all available
processors

@@ +502,3 @@
+      // TODO(fancycode): is 24/1 a sane default or can we get it from the
container somehow?
+      GST_WARNING ("Framerate is too high (%d/%d), defaulting to 24/1",
+          state->info.fps_n, state->info.fps_d);

You get the framerate from the caps on the sinkpad if known. Or 0/1 if variable
framerate. Don't assume any values or set defaults

@@ +506,3 @@
+      state->info.fps_d = 1;
+    }
+    gst_video_decoder_negotiate (decoder);

This can fail :)

@@ +513,3 @@
+    GST_DEBUG ("Frame dimensions are %d x %d", width, height);
+    dec->width = width;
+    dec->height = height;

If you store the output state it contains state->info.width and .height. No
need to store it separately

@@ +687,3 @@
+
+  GST_VIDEO_CODEC_FRAME_FLAG_SET (frame,
+      GST_VIDEO_CODEC_FRAME_FLAG_DECODE_ONLY);

Why?

@@ +729,3 @@
+          ("Error while flushing data: %s (code=%d)",
+              de265_get_error_text (ret), ret), (NULL));
+      return GST_FLOW_ERROR;

You forget unmapping the buffer here

@@ +792,3 @@
+  }
+
+  result = gst_video_decoder_allocate_output_frame (decoder, frame);

This uses the input frame for output if I'm not mistaken... which is going to
cause problems if the input is not in presentation order.

Also how do you handle the case that the decoder outputs multiple frames at
once? This doesn't seem to be handled anywhere currently.

@@ +809,3 @@
+    int height = de265_get_image_height (img, plane);
+    const uint8_t *src = de265_get_image_plane (img, plane, &stride);
+    if (stride == width) {

You don't initialize stride here anywhere. Also you need to map the output
buffer with GstVideoFrame API and handle the plane offsets and per-plane
strides correctly

::: ext/libde265/libde265-dec.h
@@ +39,3 @@
+  GST_TYPE_LIBDE265_DEC_PACKETIZED,
+  GST_TYPE_LIBDE265_DEC_RAW
+} GstLibde265DecMode;

Indention seems to be confused here

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.