Bug 122996 – [GStreamer][GTK] Add GRefPtr::outPtr()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brendan Long]

I keep getting requests in reviews to use GRefPtr in cases like this:

    GstObject* x;
    g_object_get(y, "some-property", &x, NULL);
    /* stuff */
    gst_object_unref(x);

The problem is that to use a GRefPtr, we'd need to do:

    GstObject* xRawPtr;
    g_object_get(y, "some-property", &xRawPtr, NULL);
    GRefPtr<GstObject> x = adoptGRef(xRawPtr);
    /* stuff */

Which is really awkward.

This patch adds this to GRefPtr:

    T*& outPtr()
    {
        ASSERT(!m_ptr);
        return m_ptr;
    }

Which lets me do this:

    GRefPtr<GstObject> x;
    g_object_get(y, "some-property", &x.outPtr(), NULL);
    /* stuff */

Messing with outPtr() has the same effect as adoptGRef() (no extra ref). As far as I can tell, g_object_get and related functions always ref their out parameters, so I think this is reasonably safe.

[Brendan Long]

Created attachment 214516 [details]
Patch

[Brendan Long]

Actually, I missed a couple places, and this one makes me wonder if outPtr() should just deref any existing m_ptr:

void MediaPlayerPrivateGStreamer::updateAudioSink()
{
    if (!m_playBin)
        return;

    GstElement* sinkPtr = 0;

    g_object_get(m_playBin.get(), "audio-sink", &sinkPtr, NULL);
    m_webkitAudioSink = adoptGRef(sinkPtr);

}

With my current patch, I could do:

void MediaPlayerPrivateGStreamer::updateAudioSink()
{
    if (!m_playBin)
        return;

    m_webkitAudioSink.clear();
    g_object_get(m_playBin.get(), "audio-sink", &m_webkitAudioSink.outPtr(), NULL);
}

But if I changed outPtr to this:

    T*& outPtr()
    {
        if (m_ptr) {
            derefGPtr(ptr);
            m_ptr = 0; /* to prevent incorrect usage */
        }
        return m_ptr;
    }

I could do:

void MediaPlayerPrivateGStreamer::updateAudioSink()
{
    if (!m_playBin)
        return;

    g_object_get(m_playBin.get(), "audio-sink", &m_webkitAudioSink.outPtr(), NULL);
}

[Brendan Long]

Created attachment 214530 [details]
Patch

I added more cases where this is useful (previously I searched for _unref, this time I searched for g_object_get).

[Brendan Long]

Oh, and I didn't change outPtr(), since I think it's valuable for it to have the same semantics as GOwnPtr::outPtr().

[Philippe Normand]

Comment on attachment 214530 [details]
Patch

Neat!

[WebKit Commit Bot]

Comment on attachment 214530 [details]
Patch

Clearing flags on attachment: 214530

Committed r157624: <http://trac.webkit.org/changeset/157624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brendan Long]

At some point it may be useful to make outPtr() implicitly clear() the internal pointer, since it would also make any usage of strings much nicer, letting change this:

        GstTagList* tags = 0;
        gst_event_parse_tag(event.get(), &tags);
        ASSERT(tags);

        gchar* tagValue;
        if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &tagValue)) {
            INFO_MEDIA_MESSAGE("Audio track %d got title %s.", m_index, tagValue);
            label = tagValue;
            g_free(tagValue);
        }

        if (gst_tag_list_get_string(tags, GST_TAG_LANGUAGE_CODE, &tagValue)) {
            INFO_MEDIA_MESSAGE("Audio track %d got language %s.", m_index, tagValue);
            language = tagValue;
            g_free(tagValue);
        }

To this:

        GstTagList* tags = 0;
        gst_event_parse_tag(event.get(), &tags);
        ASSERT(tags);

        GOwnPtr<gchar> tagValue;
        if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &tagValue.outPtr())) {
            INFO_MEDIA_MESSAGE("Audio track %d got title %s.", m_index, tagValue);
            label = tagValue;
        }

        if (gst_tag_list_get_string(tags, GST_TAG_LANGUAGE_CODE, &tagValue.outPtr())) {
            INFO_MEDIA_MESSAGE("Audio track %d got language %s.", m_index, tagValue);
            language = tagValue;
        }