Bug 114629 - [Mac] in-band cues sometimes displayed late
Summary: [Mac] in-band cues sometimes displayed late
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-04-15 11:42 PDT by Eric Carlson
Modified: 2013-04-16 13:53 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (33.23 KB, patch)
2013-04-15 13:12 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (33.07 KB, patch)
2013-04-15 13:40 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-04-15 11:42:20 PDT
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.
Comment 1 Eric Carlson 2013-04-15 13:11:34 PDT
<rdar://problem/13434762>
Comment 2 Eric Carlson 2013-04-15 13:12:27 PDT
Created attachment 198175 [details]
Proposed patch
Comment 3 WebKit Commit Bot 2013-04-15 13:16:01 PDT
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.
Comment 4 Eric Carlson 2013-04-15 13:40:16 PDT
Created attachment 198178 [details]
Updated patch.
Comment 5 Jer Noble 2013-04-16 10:59:41 PDT
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.
Comment 6 Eric Carlson 2013-04-16 13:52:58 PDT
https://trac.webkit.org/r148540