[Webkit-unassigned] [Bug 78883] [GStreamer] AudioSourceProvider support in the MediaPlayer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78883





--- Comment #25 from Philippe Normand <pnormand at igalia.com>  2013-01-19 01:30:16 PST ---
(In reply to comment #24)
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:167
> > +#ifndef GST_API_VERSION_1
> > +    if (m_audioSinkBin) {
> > +        gst_object_unref(m_audioSinkBin);
> > +        m_audioSinkBin = 0;
> > +    }
> > +    gst_buffer_list_iterator_free(m_frontLeftBuffersIterator);
> > +    gst_buffer_list_iterator_free(m_frontRightBuffersIterator);
> > +#endif
> > +    gst_buffer_list_unref(m_frontLeftBuffers);
> > +    gst_buffer_list_unref(m_frontRightBuffers);
> 
> I still don't really understand why m_audioSinkBin isn't a RefPtr or unreffed for Gstreamer 0.1. I think that the not-unreffing thing deserves at least a comment explaining why.
> 

I wonder now why this is in the 0.10 code path indeed.
We can't use a GRefPtr for the GstBin because gst_bin_new returns a floating reference :/ So a Debug build would ASSERT in the adoptGRef call.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:193
> > +    GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure);
> 
> You could use a GOwnPtr here.
> 

Well that would mean adding a GOwnPtrGStreamer module again likely only for this gst 0.10 API. I'm not sure it's worth it.

Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:409
> > +    bool done = false;
> > +    while (!done) {
> > +#ifndef GST_API_VERSION_1
> > +        gpointer data;
> > +
> > +        switch (gst_iterator_next(iter, &data)) {
> > +        case GST_ITERATOR_OK: {
> > +            GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
> > +            cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
> > +            break;
> > +        }
> > +#else
> > +        GValue item = G_VALUE_INIT;
> > +        switch (gst_iterator_next(iter, &item)) {
> > +        case GST_ITERATOR_OK: {
> > +            GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
> > +            cleanUpElementsAfterDeinterleaveSourcePad(pad);
> > +            break;
> > +        }
> > +#endif
> > +        case GST_ITERATOR_RESYNC:
> > +            gst_iterator_resync(iter);
> > +            break;
> > +        case GST_ITERATOR_ERROR:
> > +            done = true;
> > +            break;
> > +        case GST_ITERATOR_DONE:
> > +            done = true;
> > +            break;
> > +        }
> > +#ifdef GST_API_VERSION_1
> 
> I think maybe you can simplify this a bit:
> 
> #ifndef GST_API_VERSION_1
> gpointer item;
> #else
> GValue item = G_VALUE_INIT;
> #endif
> 
> 
> GstIteratorResult result = gst_iterator_next(iter, &item)
> while (result != GST_ITERATOR_ERROR && result != GST_ITERATOR_DONE) {
>     if (result == GST_ITERATOR_OK) {
> #ifndef GST_API_VERSION_1
>     GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
>      cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
> #else
>     GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
>     cleanUpElementsAfterDeinterleaveSourcePad(pad);
> #endif
>     } elseif (result == GST_ITERATOR_RESYNC
>         gst_iterator_resync(iter);
>    else
>        ASSERT(result == GST_ITERATOR_DONE || result == GST_ITERATOR_ERROR);
> }
> 
> It seems like you are also missing some error handling here?
> 

Actually I'm not sure what to do in case of GST_ITERATOR_ERROR in this context...

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:410
> > +        g_value_unset(&item);
> 
> Do you only need to unset the value at the end, only when result == GST_ITERATOR_OK, or every iteration?
> 

It should be needed only if result = GST_ITERATOR_OK but I guess it's acceptable to do it at every iteration.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.