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

[bugzilla-daemon at webkit.org]

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


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #209897|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #132 from Eric Carlson <eric.carlson at apple.com>  2013-08-28 10:54:57 PST ---
(From update of attachment 209897)
View in context: https://bugs.webkit.org/attachment.cgi?id=209897&action=review

I think this is close, but you definitely need to fix the layering violation. Please consider the other comments as well.

> Source/WebCore/ChangeLog:20
> +           media/track/in-band/track-in-band-kate-ogg-cues-added-once.html
> +           media/track/in-band/track-in-band-kate-ogg-kind.html
> +           media/track/in-band/track-in-band-kate-ogg-language.html
> +           media/track/in-band/track-in-band-kate-ogg-mode.html
> +           media/track/in-band/track-in-band-kate-ogg-style.html
> +           media/track/in-band/track-in-band-kate-ogg-track-order.html
> +           media/track/in-band/track-in-band-srt-mkv-cues-added-once.html
> +           media/track/in-band/track-in-band-srt-mkv-kind.html
> +           media/track/in-band/track-in-band-srt-mkv-language.html
> +           media/track/in-band/track-in-band-srt-mkv-mode.html
> +           media/track/in-band/track-in-band-srt-mkv-style.html
> +           media/track/in-band/track-in-band-srt-mkv-track-order.html

It *looks* like the kate and mkv feature tests are identical except for the media file. If so, please include the scripts from a .js file instead of duplicating them.

> Source/WebCore/html/track/InbandTextTrack.cpp:406
> +void InbandTextTrack::inbandTextTrackPrivateLabelChanged(InbandTextTrackPrivate* trackPrivate)

Nit: this is a method on InbandTextTrackPrivateClient so "inbandTextTrackPrivate" seems unnecessary in the method name.

Why require the track to call back into the media engine to get the label instead of passing it as a parameter?

> Source/WebCore/html/track/InbandTextTrack.cpp:413
> +void InbandTextTrack::inbandTextTrackPrivateLanguageChanged(InbandTextTrackPrivate* trackPrivate)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:171
> +        gboolean ret = gst_buffer_map(buffer, &info, GST_MAP_READ);
> +        ASSERT_UNUSED(ret, ret);

Is it worth have a "continue" if ret is non-zero in a release build.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:234
> +    m_cues.appendVector(cues);
> +    for (size_t i = 0; i < cues.size(); ++i)
> +        client()->addWebVTTCue(this, cues[i]);

Won't this re-add cues you have already added unless you clear m_cues?

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:84
> +    OwnPtr<WebVTTParser> m_webVTTParser;

The WebVTT parser is in html/track/ so this is a layering violation :-(. Luckily, I don't think it would be a big deal to pass the raw VTT bytes through the client interface and have the track do the parsing. It might make sense to subclass InbandTextTrack so every in-band track doesn't have get a VTT parser (I maybe should have done the same for "Generic" cues).

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:577
> +#if ENABLE(VIDEO_TRACK) && defined(GST_API_VERSION_1)
> +    /* Clear cues so we don't get duplicates */
> +    for (size_t i = 0; i < m_textTracks.size(); ++i)
> +        m_textTracks[i]->clearCues();
> +#endif

Do you really want to clear all in-band cues every time you seek? AVFoundation also delivers re-cues after a seek, but instead of flushing on a seek I just don't re-add a cue if it already exists, see InbandTextTrack::addGenericCue. This does mean that a track can't have two *identical* cues, but seems like an uncommon enough situation that it is worth the risk.

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