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

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #9 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-08-28 10:29:37 UTC ---
(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 :)

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