Bug 107046 – Support non-WebVTT cues from in-band text tracks

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

[Eric Carlson]

r137161 added support for in-band text tracks, and to created WebVTT cues from other types of cue. This translation is not always successful, so we should have a "generic" cue type internal to WebCore which does not use the WebVTT rendering rules.

[Eric Carlson]

Created attachment 183034 [details]
Proposed patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/track/TextTrackCueGeneric.cpp:54:  The parameter name "document" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/track/TextTrackCueGeneric.cpp:54:  The parameter name "cue" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:302:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/html/track/TextTrackCueGeneric.h:26:  #ifndef header guard has wrong style, please use: TextTrackCueGeneric_h  [build/header_guard] [5]
Source/WebCore/html/track/TextTrackCueGeneric.h:69:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/track/TextTrack.h:108:  The parameter name "cue" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 17 files


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

[Eric Carlson]

Created attachment 183037 [details]
Update patch for my good friend the style bot.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/track/TextTrack.h:108:  The parameter name "cue" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 17 files


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

[WebKit Review Bot]

Comment on attachment 183037 [details]
Update patch for my good friend the style bot.

Attachment 183037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15914508

[Eric Carlson]

Created attachment 183050 [details]
Add new files to other port build systems.

[EFL EWS Bot]

Comment on attachment 183050 [details]
Add new files to other port build systems.

Attachment 183050 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15909617

[Eric Carlson]

Created attachment 183074 [details]
Another patch, another build system.

[Sam Weinig]

Comment on attachment 183074 [details]
Another patch, another build system.

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

r- due to the leak.  I am also not fond of the name TextTrackCueGeneric, since I don't understand what is generic about it.

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:103
> +    :TextTrackCue(context, start, end, content)

Missing space.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:50
> +    enum alignment {

Types should start with a capital letter.

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:335
> +    m_cues.resize(0);

This looks like a leak to me.

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66
> +    Vector<GenericCueData*> m_cues;

I don't see anyone deleting these cues.  Can we use an OwnPtr?

[Eric Carlson]

(In reply to comment #9)
> (From update of attachment 183074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183074&action=review
> 
> r- due to the leak.  I am also not fond of the name TextTrackCueGeneric, since I don't understand what is generic about it.
> 
A "generic" cue is a non-WebVTT cue, so it is one which should not be positioned/sized as if it was WebVTT. I agree that TextTrackCueGeneric isn't a great name, but I couldn't come up with anything better (TextTrackCueNonWebVTT, yuck). I added a short comment explaining what it is.

> > Source/WebCore/html/track/TextTrackCueGeneric.cpp:103
> > +    :TextTrackCue(context, start, end, content)
> 
> Missing space.
> 
Fixed.

> > Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:50
> > +    enum alignment {
> 
> Types should start with a capital letter.
> 
Fixed.

> > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:335
> > +    m_cues.resize(0);
> 
> This looks like a leak to me.
> 
> > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66
> > +    Vector<GenericCueData*> m_cues;
> 
> I don't see anyone deleting these cues.  Can we use an OwnPtr?
>
Oops, it was a leak. Fixed by changing to Vector<OwnPtr<GenericCueData> >.

[Eric Carlson]

Created attachment 183826 [details]
Updated patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:307:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:327:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 23 files


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

[Eric Carlson]

Created attachment 183828 [details]
Updated patch.

[Sam Weinig]

Comment on attachment 183828 [details]
Updated patch.

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

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:95
> +private:
> +
> +    double m_startTime;

Unnecessary newline.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:307:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 23 files


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

[Eric Carlson]

Created attachment 183831 [details]
One byte smaller this time.

[Eric Carlson]

Comment on attachment 183831 [details]
One byte smaller this time.

http://trac.webkit.org/changeset/140359

[Radar WebKit Bug Importer]

<rdar://problem/13055333>