Bug 122996 - [GStreamer][GTK] Add GRefPtr::outPtr()
Summary: [GStreamer][GTK] Add GRefPtr::outPtr()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brendan Long
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-17 14:39 PDT by Brendan Long
Modified: 2013-10-18 10:14 PDT (History)
13 users (show)

See Also:


Attachments
Patch (6.61 KB, patch)
2013-10-17 14:47 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (12.68 KB, patch)
2013-10-17 16:25 PDT, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Long 2013-10-17 14:39:25 PDT
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.
Comment 1 Brendan Long 2013-10-17 14:47:23 PDT
Created attachment 214516 [details]
Patch
Comment 2 Brendan Long 2013-10-17 15:02:49 PDT
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);
}
Comment 3 Brendan Long 2013-10-17 16:25:04 PDT
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).
Comment 4 Brendan Long 2013-10-17 16:25:55 PDT
Oh, and I didn't change outPtr(), since I think it's valuable for it to have the same semantics as GOwnPtr::outPtr().
Comment 5 Philippe Normand 2013-10-17 23:50:40 PDT
Comment on attachment 214530 [details]
Patch

Neat!
Comment 6 WebKit Commit Bot 2013-10-18 00:15:53 PDT
Comment on attachment 214530 [details]
Patch

Clearing flags on attachment: 214530

Committed r157624: <http://trac.webkit.org/changeset/157624>
Comment 7 WebKit Commit Bot 2013-10-18 00:15:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Brendan Long 2013-10-18 10:14:26 PDT
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;
        }