[Webkit-unassigned] [Bug 103771] [GStreamer] support in-band text tracks

[bugzilla-daemon at webkit.org]

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





--- Comment #150 from Brendan Long <b.long at cablelabs.com>  2013-08-30 10:59:43 PST ---
I made most of the changes suggested, except:

(In reply to comment #149)
> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:110
> > +    if (m_tagTimerHandler)
> > +        g_source_remove(m_tagTimerHandler);
> 
> Should you zero m_tagTimerHandler as well?

It doesn't really matter because that function starts with:

    if (m_isDisconnected)
        return;

    m_isDisconnected = true;

I think we don't actually need m_isDisconnected anymore though (I think it was originally useful for something else), so I changed it to this:

    if (!m_pad)
        return;

Which has the same effect.

> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:201
> > +        m_label = emptyAtom;
> > +        m_language = emptyAtom;
> 
> If the track previously had a label/language, InbandTextTrack will now be out of sync.

I fixed this so it sends events when the label and language change, instead of every time we see non-empty tags:

    String label;
    String language;
    if (tags) {
        gchar* tagValue;
        if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &tagValue)) {
            INFO_MEDIA_MESSAGE("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("Track %d got language %s.", m_index, tagValue);
            language = tagValue;
            g_free(tagValue);
        }
    }

    if (m_label != label) {
        m_label = label;
        client()->labelChanged(this, m_label);
    }

    if (m_language != language) {
        m_language = language;
        client()->languageChanged(this, m_language);
    }

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:645
> > +    m_textTimerHandler = 0;
> 
> Do you not need to call g_source_remove() here as well?

No, `mediaPlayerPrivateTextChangeTimeoutCallback` returns FALSE. From the documentation for g_timeout_add:

> The function is called repeatedly until it returns FALSE, at which point the timeout is automatically destroyed and the function will not be called again. 

For these functions, I followed what was already being done in MediaPlayerPrivateGStreamer, but it's possible that this:

    void MediaPlayerPrivateGStreamer::textChanged()
    {
        if (m_textTimerHandler)
            g_source_remove(m_textTimerHandler);
        m_textTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateTextChangeTimeoutCallback), this);
    }

Could be this:

    void MediaPlayerPrivateGStreamer::textChanged()
    {
        if (!m_textTimerHandler)
            m_textTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateTextChangeTimeoutCallback), this);
    }

I *think* rescheduling the event is useful though, since we tend to get all of the tracks at once, and pushing the event back every time we see one lets us consolidate all of those into one event.

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