Bug 107046

Summary: Support non-WebVTT cues from in-band text tracks
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dino, feature-media-reviews, gyuyoung.kim, ojan.autocc, rakuco, sam, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Proposed patch
none
Update patch for my good friend the style bot.
webkit.review.bot: commit-queue-
Add new files to other port build systems.
eflews.bot: commit-queue-
Another patch, another build system.
sam: review-
Updated patch.
none
Updated patch.
sam: review+
One byte smaller this time. none

Eric Carlson
Reported 2013-01-16 13:14:57 PST
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.
Attachments
Proposed patch (60.73 KB, patch)
2013-01-16 14:18 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Update patch for my good friend the style bot. (60.70 KB, patch)
2013-01-16 14:52 PST, Eric Carlson 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Add new files to other port build systems. (63.07 KB, patch)
2013-01-16 15:53 PST, Eric Carlson 		eflews.bot: commit-queue-
DetailsFormatted DiffDiff
Another patch, another build system. (63.55 KB, patch)
2013-01-16 17:55 PST, Eric Carlson 		sam: review-
DetailsFormatted DiffDiff
Updated patch. (67.49 KB, patch)
2013-01-21 13:24 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Updated patch. (67.44 KB, patch)
2013-01-21 13:44 PST, Eric Carlson 		sam: review+
DetailsFormatted DiffDiff
One byte smaller this time. (67.44 KB, patch)
2013-01-21 13:51 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Eric Carlson
Comment 1 2013-01-16 14:18:51 PST
Created attachment 183034 [details] Proposed patch
WebKit Review Bot
Comment 2 2013-01-16 14:21:57 PST
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
Comment 3 2013-01-16 14:52:26 PST
Created attachment 183037 [details] Update patch for my good friend the style bot.
WebKit Review Bot
Comment 4 2013-01-16 14:54:03 PST
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 5 2013-01-16 15:41:02 PST
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
Comment 6 2013-01-16 15:53:22 PST
Created attachment 183050 [details] Add new files to other port build systems.
EFL EWS Bot
Comment 7 2013-01-16 16:58:06 PST
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
Comment 8 2013-01-16 17:55:16 PST
Created attachment 183074 [details] Another patch, another build system.
Sam Weinig
Comment 9 2013-01-20 14:45:36 PST
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
Comment 10 2013-01-21 13:23:11 PST
(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
Comment 11 2013-01-21 13:24:04 PST
Created attachment 183826 [details] Updated patch.
WebKit Review Bot
Comment 12 2013-01-21 13:29:55 PST
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
Comment 13 2013-01-21 13:44:45 PST
Created attachment 183828 [details] Updated patch.
Sam Weinig
Comment 14 2013-01-21 13:47:23 PST
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
Comment 15 2013-01-21 13:49:15 PST
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
Comment 16 2013-01-21 13:51:25 PST
Created attachment 183831 [details] One byte smaller this time.
Eric Carlson
Comment 17 2013-01-21 14:05:49 PST
Comment on attachment 183831 [details] One byte smaller this time. http://trac.webkit.org/changeset/140359
Radar WebKit Bug Importer
Comment 18 2013-01-21 14:07:15 PST
<rdar://problem/13055333>
Note You need to log in before you can comment on or make changes to this bug.