[Bug 726325] RFC: Add tunneling support.

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=726325
  GStreamer | gst-omx | git

--- Comment #25 from deathsimple at vodafone.de <deathsimple at vodafone.de> 2014-03-19 11:13:15 UTC ---
(In reply to comment #16)
> Review of attachment 271868 [details]:
> 
> ::: omx/gstomxvideodec.c
> @@ +51,3 @@
>  #include "gstomxvideo.h"
>  #include "gstomxvideodec.h"
> +#include "gstomxvideoenc.h"
> 
> I guess this is a residual of your initial try and then you forgot to remove it
> ? Otherwise I do not see why it's useful

Yeah, just a leftover. Fixed by now.

> 
> @@ +1146,3 @@
>    gst_video_codec_state_unref (state);
> 
> +  pool = gst_video_decoder_get_buffer_pool (GST_VIDEO_DECODER (self));
> 
> I think you never unref the pool
> (http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-GstVideoDecoder.html#gst-video-decoder-get-buffer-pool)

Thx, fixed this as well.

> @@ +1154,3 @@
> +    if (gst_omx_setup_tunnel (self->dec_out_port, op->port) != OMX_ErrorNone)
> +      GST_WARNING_OBJECT (self, "Failed to setup tunnel");
> +  }
> 
> will it fallback to non-tunneling mode if it fails to setup a tunnel ?

Yes, that's the plan.

> 
> @@ +2230,3 @@
> +  if (self->dec_out_port->tunneled) {
> +    frame->output_buffer = gst_buffer_new ();
> +    gst_video_decoder_finish_frame (GST_VIDEO_DECODER (self), frame);
> 
> Try to just use gst_video_decoder_release_frame
> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-GstVideoDecoder.html#gst-video-decoder-release-frame
> 
> Because gst_video_decoder_finish_frame actually push the data.
> 
> Note that on the gstomxencoder side you do need to have
> gst_omx_video_enc_handle_frame being called. In your other patch you just unref
> the frame. Better to no have the chain to be called at all in case of
> GST_PAD_PROBE_TYPE_BUFFER + tunnel

So far that didn't worked for various reasons, but I'm already working on
cleaning this up as well.

> @@ +2367,3 @@
>      GST_ERROR_OBJECT (self, "Failed to drain component: %s (0x%08x)",
>          gst_omx_error_to_string (err), err);
> +    g_mutex_unlock (&self->drain_lock);
> 
> please put this in another patch

Done.

> @@ +2381,3 @@
> +        g_cond_wait_until (&self->drain_cond, &self->drain_lock, wait_until);
> +
> +    if (!result)
> 
> please put this in another patch

That's actually an unnecessary change, going to remove it from the patch.

> @@ +2395,3 @@
>    GST_VIDEO_DECODER_STREAM_LOCK (self);
> 
>    self->started = FALSE;
> 
> Is it possible to avoid starting the GstTask when tunneling
> http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n1937 ?
> I mean is gst_omx_video_dec_loop necessary
> http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n1161 ?

So far my current thinking is to still start the task so that it can react to
errors send by the component.

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