[Bug 725145] libde265 based HEVC/H.265 decoder plugin
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Thu Aug 28 02:49:40 PDT 2014
https://bugzilla.gnome.org/show_bug.cgi?id=725145
GStreamer | gst-plugins-bad | git
--- Comment #8 from Joachim Bauch <jojo at struktur.de> 2014-08-28 09:49:34 UTC ---
(In reply to comment #7)
> Review of attachment 284591 [details]:
Thanks for the detailed review. Some feedback below, I will update the patch
once I did all the changes.
> 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
As the library is called "libde265" (not "de265"), I was following the naming
scheme of "libfame" where the file is "gstlibfame.c".
> ::: 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
ok
> ::: 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
ok
> @@ +51,3 @@
> +
> +// use two decoder threads if no information about
> +// available CPU cores can be retrieved
>
> No C99 comments
ok
> @@ +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
ok
> @@ +66,3 @@
> + GST_PAD_SRC,
> + GST_PAD_ALWAYS,
> + GST_STATIC_CAPS (GST_VIDEO_CAPS_MAKE ("{ I420 }"))
>
> You can omit the {} here
ok
> @@ +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
ok
> @@ +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
ok
> @@ +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
ok. Is "GST_ELEMENT_ERROR (decoder, STREAM, DECODE, ...)" fine for errors
during decoding, or should I also change these to GST_ERROR_OBJECT?
> @@ +255,3 @@
> + GstVideoFrame vframe;
> + GstBuffer *buffer;
> + int mapped;
>
> gboolean
ok
> @@ +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()
ok
> @@ +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?
This function is called from the decoder, so it needs to access the current
frame. I was looking into gst-ffmpeg as a reference how direct rendering should
be implemented. Is there a better way to do this?
> @@ +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
Sounds good, will change.
> @@ +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
ok
> @@ +506,3 @@
> + state->info.fps_d = 1;
> + }
> + gst_video_decoder_negotiate (decoder);
>
> This can fail :)
Right :) will add a check.
> @@ +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
ok
> @@ +687,3 @@
> +
> + GST_VIDEO_CODEC_FRAME_FLAG_SET (frame,
> + GST_VIDEO_CODEC_FRAME_FLAG_DECODE_ONLY);
>
> Why?
Saw that in some other plugin (can't remember now) but you're right, that is
not necessary. Will remove.
> @@ +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
thanks, will change
> @@ +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.
As before I was following gst-ffmpeg on this. I have a couple of streams for
testing where the input is not in presentation order, but don't see any obvious
problems.
> Also how do you handle the case that the decoder outputs multiple frames at
> once? This doesn't seem to be handled anywhere currently.
Decoding stops once a picture is available from the decoder and the remaining
data is cached in the decoder, so any additional images will be processed in
the next decoding loop.
> @@ +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
Will add an explicit initialization for "stride" (doesn't really matter as the
variable is always set in the codepath of "de265_get_image_plane" in libde265).
The function is processing a GstVideoCodecFrame, so I probably can't use the
GstVideoFrame API, but will have to get the offsets/strides from the output
format.
> ::: ext/libde265/libde265-dec.h
> @@ +39,3 @@
> + GST_TYPE_LIBDE265_DEC_PACKETIZED,
> + GST_TYPE_LIBDE265_DEC_RAW
> +} GstLibde265DecMode;
>
> Indention seems to be confused here
Strange, that was auto-indented using gst-indent, will change.
--
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.
More information about the gstreamer-bugs
mailing list