Bug 187636 – [GStreamer][MSE] Add GstFlowCombiner to handle non-linked inactive branches

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[GStreamer][MSE] Add GstFlowCombiner to handle non-linked inactive branches

[Xabier Rodríguez Calvar]

Created attachment 344930 [details]
Patch

[Enrique Ocaña]

This patch looks good to me. It's going to be very helpful to support more than one audio streams.

[Carlos Garcia Campos]

Comment on attachment 344930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344930&action=review

> Source/WebCore/platform/graphics/gstreamer/GUniquePtrGStreamer.h:36
> +// GstFlowCombiner becomes reference counted in 1.12.

What does this comment mean? It's confusing, if it's ref counted, why we define a GUniquePtr?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:255
> +    return gst_flow_combiner_update_pad_flow(self.get()->priv->flowCombiner.get(), pad, gst_proxy_pad_chain_default(pad, GST_OBJECT(self.get()), buffer));

self.get()->priv -> self->priv

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:482
> -    GUniquePtr<gchar> padName(g_strdup_printf("src_%u", padId));
> -    GstPad* ghostpad = WebCore::webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.get(), sourcePad);
> +    auto padName = String::format("src_%u", padId);
> +    GstPad* ghostPad = WebCore::webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.utf8().data(), sourcePad);

Why did you change this? We are now doing encoding conversions.

[Xabier Rodríguez Calvar]

(In reply to Carlos Garcia Campos from comment #3)
> > Source/WebCore/platform/graphics/gstreamer/GUniquePtrGStreamer.h:36
> > +// GstFlowCombiner becomes reference counted in 1.12.
> 
> What does this comment mean? It's confusing, if it's ref counted, why we
> define a GUniquePtr?

It became refcounted in 1.12 so from that moment on can can ref and unref but with previous versions of GStreamer, we have _new and _free. Since we don't need it to be refcounted for now, I thought the simplest way was implementing the non-refcounted use case.

> > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:255
> > +    return gst_flow_combiner_update_pad_flow(self.get()->priv->flowCombiner.get(), pad, gst_proxy_pad_chain_default(pad, GST_OBJECT(self.get()), buffer));
> 
> self.get()->priv -> self->priv

Yes (monkey coding).

> > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:482
> > -    GUniquePtr<gchar> padName(g_strdup_printf("src_%u", padId));
> > -    GstPad* ghostpad = WebCore::webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.get(), sourcePad);
> > +    auto padName = String::format("src_%u", padId);
> > +    GstPad* ghostPad = WebCore::webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.utf8().data(), sourcePad);
> 
> Why did you change this? We are now doing encoding conversions.

Cause I think it was better to use WebKit API rather than GLib's if possible. I think it is better. Do you object to this?

[Carlos Garcia Campos]

(In reply to Xabier Rodríguez Calvar from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > > Source/WebCore/platform/graphics/gstreamer/GUniquePtrGStreamer.h:36
> > > +// GstFlowCombiner becomes reference counted in 1.12.
> > 
> > What does this comment mean? It's confusing, if it's ref counted, why we
> > define a GUniquePtr?
> 
> It became refcounted in 1.12 so from that moment on can can ref and unref
> but with previous versions of GStreamer, we have _new and _free. Since we
> don't need it to be refcounted for now, I thought the simplest way was
> implementing the non-refcounted use case.

Remove the comment then, it's confusing.

> > > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:255
> > > +    return gst_flow_combiner_update_pad_flow(self.get()->priv->flowCombiner.get(), pad, gst_proxy_pad_chain_default(pad, GST_OBJECT(self.get()), buffer));
> > 
> > self.get()->priv -> self->priv
> 
> Yes (monkey coding).
> 
> > > Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:482
> > > -    GUniquePtr<gchar> padName(g_strdup_printf("src_%u", padId));
> > > -    GstPad* ghostpad = WebCore::webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.get(), sourcePad);
> > > +    auto padName = String::format("src_%u", padId);
> > > +    GstPad* ghostPad = WebCore::webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.utf8().data(), sourcePad);
> > 
> > Why did you change this? We are now doing encoding conversions.
> 
> Cause I think it was better to use WebKit API rather than GLib's if
> possible. I think it is better. Do you object to this?

Yes, WTF is preferred but only when it makes sense, and it doesn't in this case. The change is unrelated to this bug too.

[Xabier Rodríguez Calvar]

Created attachment 344933 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 344933 [details]
Patch

Clearing flags on attachment: 344933

Committed r233797: <https://trac.webkit.org/changeset/233797>

[Xabier Rodríguez Calvar]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42160243>