Cool stuff, thanks for the patch.

Out of curiosity, how does your decoder implementation differ from the LGPL implementation that's part of libav?

> Does the code match the requirements for inclusion in the "good" set of plugins
> and if not, what needs to be done to do so?

Due to the patent situation -good might not be appropriate for this plugin. In general new plugins get added to gst-plugins-bad and then later moved to either -good or -ugly.

So if you could prepare a patch against -bad, that would be great.

Also, could you please ship a pkg-config .pc file with your decoder library and fix the configure check up to use that to detect/find the library + version ?

(In reply to comment #1)
> Cool stuff, thanks for the patch.

Thanks for the fast reply :)

> Out of curiosity, how does your decoder implementation differ from the LGPL
> implementation that's part of libav?

Both decoders are currently at about the same development status with comparable performance, reaching 50fps @ 4K. With libde265, the plugin depends on a smaller library and is more modular.

We are concentrating on streaming applications and want to support additional features that are not possible with a catch-all API, like dynamic switching between sub-streams with different fps. Work on a complementing encoder is currently starting.

We have ARM NEON support in the queue and are evaluating whether CUDA support makes sense.

> Due to the patent situation -good might not be appropriate for this plugin. In
> general new plugins get added to gst-plugins-bad and then later moved to either -good or -ugly.
> 
> So if you could prepare a patch against -bad, that would be great.

I see. I'm currently looking into providing a patch against -bad and will attach that here when done.

> Also, could you please ship a pkg-config .pc file with your decoder library and
> fix the configure check up to use that to detect/find the library + version ?

I added that to our TODO-list for the next release of the library and will then update the configure check accordingly.

Created attachment 270281 [details] [review]
Patch for -bad plugins

New patch to add the plugin to -bad.

Created attachment 274248 [details] [review]
Updated libde265 integration.

Changes:
- use pkg-config to check for libde265
- requires at least libde265 0.6 (latest version)
- updated to use new API provided by libde265
- release mapped buffers on error

Created attachment 274251 [details] [review]
Updated libde265 integration.

Found and fixed an issue when creating the output image and "width" != "stride" in decoded image.

Created attachment 284591 [details] [review]
Updated libde265 integration.

Updated libde265 integration.

Changes:
- rebased to master commit c93017a074f221fac0fd813f1725386ac9acce74
- updated to libde265 0.8 (latest version)
- implemented direct rendering
- improved detection / support for packetized / raw streams

libde265 is now available in Debian (Testing / Unstable) [1] and will be included in Ubuntu Utopic [2], so it would be great to get this plugin into GStreamer.

Any feedback appreciated.

[1] https://packages.qa.debian.org/libd/libde265.html
[2] http://packages.ubuntu.com/source/utopic/libde265

Review of attachment 284591 [details] [review]:

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

(In reply to comment #7)
> Review of attachment 284591 [details] [review]:

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.

(In reply to comment #8)

> > 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".

gstlibde265dec.c seems fine then.

> > @@ +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?

GST_ERROR_OBJECT() just prints something to the debug log with error level. GST_ELEMENT_ERROR() posts an error on the bus which will cause the application to shut down the pipeline.

> > @@ +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?

But that will always give the current frame number to the decoder. Is it guaranteed that it will never be called for an older frame?

> > @@ +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.

As this is in the code path without direct rendering, did you make sure it goes through that actually? The problem here is that you will output the current GstVideoCodecFrame (which has the current PTS and DTS) with an actual video frame that has a DTS before the current one, but has the PTS of the frame that should come now.

> > 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.

Where do you make sure to drain all remaining frames from the decoder at EOS?

> > @@ +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).

Oh I missed that part, sorry.

> 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.

You can use gst_video_frame_map() on the buffer that is contained in the GstVideoCodecFrame.

> > ::: 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.

gst-indent sometimes behaves weird on headers :)

(In reply to comment #9)
> > > @@ +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?
> 
> But that will always give the current frame number to the decoder. Is it
> guaranteed that it will never be called for an older frame?

Yes, as the plugin is triggering the decoding immediately after pushing data into the decoder, so it will call the "get_buffer" function for the currently decoded 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.
> 
> As this is in the code path without direct rendering, did you make sure it goes
> through that actually? The problem here is that you will output the current
> GstVideoCodecFrame (which has the current PTS and DTS) with an actual video
> frame that has a DTS before the current one, but has the PTS of the frame that
> should come now.

I also tested the code with the non-direct rendering path which runs fine with my non-presentation order streams. Shouldn't that be the case anyway as the plugin returns frames with correct PTS?

> Where do you make sure to drain all remaining frames from the decoder at EOS?

Right, I just noticed the "finish" function was missing. Will add that.

Created attachment 284725 [details] [review]
Updated libde265 integration.

Various changes based on feedback from slomo:
- No need to check libde265 version again, already done by pkg-config.
- Changed library order when linking.
- Removed unnecessary curly brackets.
- Field should be a "gboolean".
- Use "g_malloc" / "g_free".
- Use "GST_*_OBJECT" macros.
- Fixed wrong indent.
- Make sure buffer is unmapped if codec can't be flushed.
- Handle case where format negotiation failed.
- Initialize "stride" to default value.
- Remove setting GST_VIDEO_CODEC_FRAME_FLAG_DECODE_ONLY, not required.
- No C99 comments.
- Make number of threads to use configurable.
- Always get stream format / frame rate from caps, not from properties.
- Map output frame and evaluate stride in non-direct rendering path.
- Implement "finish" to drain remaining frames on end.

(In reply to comment #10)
> > > > 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.
> > 
> > As this is in the code path without direct rendering, did you make sure it goes
> > through that actually? The problem here is that you will output the current
> > GstVideoCodecFrame (which has the current PTS and DTS) with an actual video
> > frame that has a DTS before the current one, but has the PTS of the frame that
> > should come now.
> 
> I also tested the code with the non-direct rendering path which runs fine with
> my non-presentation order streams. Shouldn't that be the case anyway as the
> plugin returns frames with correct PTS?

Yes but you store the frame with the correct PTS in the wrong GstVideoCodecFrame. Consider the following case:

- input 1
- no output

- input 2
- no output

- input 3
- output 2 (stored in GstVideoCodecFrame for 3)

This will confuse the internal bookkeeping inside GstVideoDecoder. It might be enough to just get the GstVideoCodecFrame from the decoder with the corresponding PTS, assuming the input has PTS. If you have no other way of correlating input frames and output frames.

Review of attachment 284725 [details] [review]:

::: ext/libde265/libde265-dec.c
@@ +755,3 @@
+  }
+  gst_video_frame_unmap (&outframe);
+  frame->pts = (GstClockTime) de265_get_image_PTS (img);

Instead of this, get the corresponding GstVideoCodecFrame and don't overwrite the pts in the frame

@@ +769,3 @@
+  de265_error ret = DE265_OK;
+  int more = 0;
+  de265_PTS pts = (de265_PTS) frame->pts;

Note that pts can be GST_CLOCK_TIME_NONE (i.e. -1) but frame->dts could contain useful values. You're supposed to generate PTS from the DTS in that case.

Ping?

Sorry, was busy the last days preparing a new upstream release of libde265 (which was finished yesterday), but also implemented the remaining changes for this plugin.

Will update the patch later today.

Created attachment 286346 [details] [review]
Updated libde265 integration.

Changes since the last patch:
- Updated to latest upstream version 0.9
- Store decoded image in correct GstVideoCodecFrame
- Don't keep "width"/"height" separately but use from output state
- Removed unused "fps" variables
- Don't set timestamp in output GstVideoCodecFrame, now that the correct one is returned (which already has the correct PTS)

Is there anything else you want to have changed?

commit 981b5595d99436e150a5875b2f8ea6a590972006
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Oct 14 10:51:22 2014 +0200

    libde265: Change rank to SECONDARY, gst-libav has PRIMARY rank

commit 51fc68e196c2fb3d27513e453eaf3b9895409287
Author: Joachim Bauch <bauch@struktur.de>
Date:   Wed Sep 17 10:38:44 2014 +0200

    Integrate libde265 into gst-plugins-bad.