Bug 114629 – [Mac] in-band cues sometimes displayed late

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

The duration of an in-band cue in the media file is implicit - it is displayed until the next sample in its track. This means that cues are delivered to WebKit without have an end time and we use the start time of the next cue (which could be empty) as the previous cue's end time. Because of this cues are not added to a track until the duration is known.

WebKit only gets in-band cues ~2 seconds in advance of their display time, so this does not work well for cues with long durations because we don't know the duration until well after the cue is supposed to be displayed.

[Eric Carlson]

<rdar://problem/13434762>

[Eric Carlson]

Created attachment 198175 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 198175 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/html/track/TextTrack.cpp', u'Source/WebCore/html/track/TextTrack.h', u'Source/WebCore/html/track/TextTrackCue.cpp', u'Source/WebCore/html/track/TextTrackCue.h', u'Source/WebCore/html/track/TextTrackCueGeneric.cpp', u'Source/WebCore/html/track/TextTrackCueGeneric.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp']" exit_code: 1
Source/WebCore/html/track/InbandTextTrack.h:48:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:48:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:50:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:51:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:53:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:54:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/TextTrack.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/html/track/TextTrack.h:113:  The parameter name "match" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:45:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:45:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:51:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:60:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:79:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 14 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 198178 [details]
Updated patch.

[Jer Noble]

Comment on attachment 198178 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=198178&action=review

> Source/WebCore/html/track/InbandTextTrack.cpp:240
> +void InbandTextTrack::updateGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData> prpCueData)

This should take a GenericCueData*, as you're not passing ownership of the cueData parameter.

> Source/WebCore/html/track/InbandTextTrack.cpp:253
> +void InbandTextTrack::removeGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData> prpCueData)

Ditto.

> Source/WebCore/html/track/InbandTextTrack.cpp:263
> +void InbandTextTrack::removeCue(PassRefPtr<TextTrackCue> prpCue, ExceptionCode& ec)

Ditto for TextTrackCue* and cue.

> Source/WebCore/html/track/InbandTextTrack.h:82
> +    virtual void addGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) OVERRIDE;
> +    virtual void updateGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) OVERRIDE;
> +    virtual void removeGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) OVERRIDE;
> +    virtual void removeCue(PassRefPtr<TextTrackCue>, ExceptionCode&) OVERRIDE;

It makes sense to pass a PassRefPtr to addGenericCue, as you're passing ownership of the parameter.  But updateGenericCue and removeGenericCue should just take a raw GenericCueData* pointer. Same for removeCue() and TextTrackCue*.

> Source/WebCore/html/track/TextTrackCue.cpp:-313
> -    cueWillChange();
>      m_pauseOnExit = value;
> -    cueDidChange();

Presumably you want to replace these with willChange() and didChange(), not remove them entirely.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:134
> +    virtual void addGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) = 0;
> +    virtual void updateGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) = 0;
> +    virtual void removeGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) = 0;

Same here as for TextTrack.

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:388
> +        client()->addGenericCue(this, cueData);

You should pass cueData.release() to addGenericCue() since it takes a PassRefPtr.

[Eric Carlson]

https://trac.webkit.org/r148540