[webkit-reviews] review denied: [Bug 103771] [GStreamer] support in-band text tracks : [Attachment 210033] Also add Windows build

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 30 10:16:04 PDT 2013


Eric Carlson <eric.carlson at apple.com> has denied Brendan Long
<b.long at cablelabs.com>'s request for review:
Bug 103771: [GStreamer] support in-band text tracks
https://bugs.webkit.org/show_bug.cgi?id=103771

Attachment 210033: Also add Windows build
https://bugs.webkit.org/attachment.cgi?id=210033&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210033&action=review


The only real problem I see is the unnecessary map in InbandWebVTTTextTrack,
but I think that makes it worth going one more round.

> Source/WebCore/ChangeLog:73
> +	   (WTF::adoptGRef):
> +	   (WTF::GstSample):
> +	   (WTF::GstEvent):

Nit: entries that don't need a comment should be removed.

> Source/WebCore/ChangeLog:77
> +	   (webkitGstCheckVersion):

Ditto.

> Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:96
> +    , m_webVTTParser(WebVTTParser::create(this, context))

The track might not ever get any cue data so why don't you allocate the parser
the first time parseWebVTTCueData is called?

> Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:105
> +void InbandWebVTTTextTrack::parseWebVTTCueData(InbandTextTrackPrivate*
trackPrivate,
> +    const char* data, unsigned length)

Nit: the line wrap it unnecessary.

> Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:135
> +	   m_cueMap.add(cueData.get(), cue.get());

I don't think you need a cue map for this class. With "generic" cues, we don't
know a cue's duration until the next cue (or empty cue) is delivered. We don't
want to delay adding them to the cue tree, or we might not display them on time
(or at all), so they are initially delivered with an infinite duration and then
updateGenericCue() is called when we know the duration. This means we need to
have a map to find the TextTrackCue that is already in the tree from the
GenericCueData, and thus the map. A cue is removed from the map ocne it gets a
duration.

In this case you have the entire WebVTT cue so will always have the duration
and the map should be unnecessary.

>> Source/WebCore/html/track/InbandWebVTTTextTrack.h:56
>> +};
> 
> It might be nice to templatize this class, since it's essentially duplicated
in both this and InbandGenericTextTrack.

Or remove it completely ;-)

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:145
> +    virtual void labelChanged(InbandTextTrackPrivate*, const String& label)
= 0;
> +    virtual void languageChanged(InbandTextTrackPrivate*, const String&
language) = 0;

Nit: these variable names aren't really necessary.

>
Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:
110
> +    if (m_tagTimerHandler)
> +	   g_source_remove(m_tagTimerHandler);

Should you zero m_tagTimerHandler as well?

>
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.

>
Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:
212
> +	   m_label = emptyAtom;

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:
220
> +	   m_language = emptyAtom;

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:645
> +    m_textTimerHandler = 0;

Do you not need to call g_source_remove() here as well?

> LayoutTests/ChangeLog:11
> +	   (.canplaythrough):
> +	   (testAttribute):

These empty entries can be removed.

> LayoutTests/ChangeLog:22
> +	   * media/track/in-band/test-cues-added-once.js: Added.
> +	   (testCuesAddedOnce):
> +	   * media/track/in-band/test-mode.js: Added.
> +	   (testMode.seeked):
> +	   (testMode.canplaythrough):
> +	   (testMode):
> +	   * media/track/in-band/test-style.js: Added.
> +	   (testStyle.seeked):
> +	   (testStyle.canplaythrough):
> +	   (testStyle):
> +	   * media/track/in-band/test-track-order.js: Added.

Each of these .js files only has a single function so they could be combined
into a single "in-band-cues.js" (or whatever) file. It should probably go into
media/ along with the other .js files used for media tests.


More information about the webkit-reviews mailing list