Bug 113522

Summary: [Qt] Enable text tracks from track elements
Product: WebKit Reporter: Brendan Long <b.long>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, b.long, commit-queue, jturcotte, menard, pnormand, vestbo, webkit.review.bot
Priority: P2 Keywords: HTML5, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 113965    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Output of running run-webkit-tests LayoutTests/media/track/* without my patch
none
Output of running run-webkit-tests LayoutTests/media/track/* with my patch
none
Patch
none
Patch
none
Fix Target.pri where I mixed up HTMLTrackElement.cpp and .h
none
Patch none

Description Brendan Long 2013-03-28 11:19:59 PDT
[qt] Enable WebVTT text tracks and inband text tracks from GStreamer
Comment 1 Brendan Long 2013-03-28 11:21:45 PDT
Created attachment 195604 [details]
Patch
Comment 2 Brendan Long 2013-03-28 11:37:11 PDT
See:

https://bugreports.qt-project.org/browse/QTBUG-29613

Sorry about the large number of changes. I was planning to separate them into four patches but the webkit-patch tool doesn't seem to like that. I can break this into multiple patches or bug reports if it would be easier to review.

This patch:

  * Enables text tracks in QtWebKit (setting VIDEO_ENABLED=1, adding the TextTrack* files to the build, adding some strings).
  * Add support for inband text tracks to ports that use GStreamer (if VIDEO_ENABLED=1).
  * Fixes some bugs in text track handling (checking cues without a null check, displaying disabled cues)
  * Changes two of the layout tests to use counting-subtitled.ogv/mov instead of counting-captioned.mov.

The change to the layout tests seems possibly controversial, since it changes the original tests and doesn't use the established standard (findMediaFile). The reason I had to do this is that GStreamer can't handle any kind of captions that I can tell, so I can't make the kind == captions part work, and as far as I can tell, GStreamer doesn't support any subtitle formats that can be held in a mp4 or mov container. My solution is to add two <source>s, with the .ogv first, on the assumption that since Safari doesn't support .ogv, this shouldn't cause any problems for them.

These changes caused two previously failing layout tests to pass. Is there somewhere that I should change to make a note of that?

Since the GStreamer changes don't just effect Qt, I'd like to get someone from WebKitGTK on this as well.
Comment 3 Brendan Long 2013-03-28 11:38:11 PDT
Also, should the component be WebKit Qt or Media Elements?
Comment 4 Brendan Long 2013-03-28 11:39:45 PDT
It looks like I need to rebase my patch. I started a fetch and will reupload the patch after lunch.
Comment 5 Alexis Menard (darktears) 2013-03-28 11:58:07 PDT
Comment on attachment 195604 [details]
Patch

Clearing review flag and commit queue flag, the patch does not apply and needs a ChangeLog. You can find on WebKit wiki the contribution guidelines. Thanks for the patch btw.
Comment 6 Alexis Menard (darktears) 2013-03-28 11:59:06 PDT
(In reply to comment #5)
> (From update of attachment 195604 [details])
> Clearing review flag and commit queue flag, the patch does not apply and needs a ChangeLog. You can find on WebKit wiki the contribution guidelines. Thanks for the patch btw.

Yes you may want to split into multiple patches.
Comment 7 Brendan Long 2013-03-28 15:37:44 PDT
Created attachment 195660 [details]
Patch
Comment 8 Brendan Long 2013-03-28 15:40:15 PDT
Sorry, I'm having trouble figuring out what the upload-patch script does, since it opens an empty text file, which doesn't seem to do anything (no file name set), and then it uploads a file without the ChangeLog.

So, one more time..
Comment 9 Brendan Long 2013-03-28 15:40:25 PDT
Sorry, I'm having trouble figuring out what the upload-patch script does, since it opens an empty text file, which doesn't seem to do anything (no file name set), and then it uploads a file without the ChangeLog.

So, one more time..
Comment 10 Brendan Long 2013-04-01 10:33:10 PDT
Changing this to just out-of-band text track since my tests for in-band don't work against GStreamer 1.0 (because the Kate plugin hasn't been ported yet). I'll work on that seperately and just upload a patch for track elements for now.
Comment 11 Brendan Long 2013-04-01 12:10:47 PDT
Created attachment 196000 [details]
Patch
Comment 12 Allan Sandfeld Jensen 2013-04-01 16:43:03 PDT
Comment on attachment 196000 [details]
Patch

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

> Source/WebCore/ChangeLog:5
> +        Enable text tracks in QtWebKit.
> +
> +        [Qt] Enable text tracks from track elements

Please drop the first line and let the bug title be the commit subject.

> Source/WebCore/ChangeLog:10
> +        No new tests because they're already there. This just enables the feature.

Could you unskip the tests in LayoutTests/platform/qt/TestExpectations then?

> Tools/qmake/mkspecs/features/features.pri:120
> -    ENABLE_VIDEO_TRACK=0 \
> +    ENABLE_VIDEO_TRACK=1 \

What happens when VIDEO_TRACK is enabled and HTML5 video or GStreamer isn't?
Comment 13 Philippe Normand 2013-04-02 04:27:07 PDT
(In reply to comment #10)
> Changing this to just out-of-band text track since my tests for in-band don't work against GStreamer 1.0 (because the Kate plugin hasn't been ported yet). I'll work on that seperately and just upload a patch for track elements for now.

Is there a bug for this failing webkit test?
Comment 14 Brendan Long 2013-04-02 07:36:11 PDT
(In reply to comment #13)
> (In reply to comment #10)
> > Changing this to just out-of-band text track since my tests for in-band don't work against GStreamer 1.0 (because the Kate plugin hasn't been ported yet). I'll work on that seperately and just upload a patch for track elements for now.
> 
> Is there a bug for this failing webkit test?

The tests in TestExpectations references this bug:

https://bugs.webkit.org/show_bug.cgi?id=103769

I'm not sure how I didn't find it before. I'll submit my inband text track things there (once I figure out how to make them testable again).
Comment 15 Philippe Normand 2013-04-02 07:39:33 PDT
I suppose it can be closed as a dupe of bug 103771
Comment 16 Brendan Long 2013-04-02 09:29:03 PDT
I'm having trouble enabling the layout tests. I went into LayoutTests/platform/qt/TestExpectations and removed the Opera tests, but if I run the layout tests on that directory, it skips them unless I include --force. I also can't figure out how to enable the tests in LayoutTests/media/track, since they're not listed in that file. Are they already enabled?
Comment 17 Brendan Long 2013-04-02 11:00:38 PDT
(In reply to comment #12)

> > Source/WebCore/ChangeLog:10
> > +        No new tests because they're already there. This just enables the feature.
> 
> Could you unskip the tests in LayoutTests/platform/qt/TestExpectations then?

As far as I can tell, they weren't being skipped. Is there something I should look at besides the TestExpectations files (I checked all of the qt and generic ones just in case). The opera ones still don't work, but those don't work on any platforms. The ones that do work are in LayoutTests/media/track (not the subdirectories).

> > Tools/qmake/mkspecs/features/features.pri:120
> > -    ENABLE_VIDEO_TRACK=0 \
> > +    ENABLE_VIDEO_TRACK=1 \
> 
> What happens when VIDEO_TRACK is enabled and HTML5 video or GStreamer isn't?

Now that you point this out, I'm confused about why video works at all in QtWebkit, since I can't figure out where VIDEO_ENABLED gets set to 1.
Comment 18 Alexis Menard (darktears) 2013-04-02 11:18:51 PDT
(In reply to comment #17)
> (In reply to comment #12)
> 
> > > Source/WebCore/ChangeLog:10
> > > +        No new tests because they're already there. This just enables the feature.
> > 
> > Could you unskip the tests in LayoutTests/platform/qt/TestExpectations then?
> 
> As far as I can tell, they weren't being skipped. Is there something I should look at besides the TestExpectations files (I checked all of the qt and generic ones just in case). The opera ones still don't work, but those don't work on any platforms. The ones that do work are in LayoutTests/media/track (not the subdirectories).

From my understanding Opera suite is not yet fully working for WebKit so we can let them skipped. TestExpectations is enough. 

> 
> > > Tools/qmake/mkspecs/features/features.pri:120
> > > -    ENABLE_VIDEO_TRACK=0 \
> > > +    ENABLE_VIDEO_TRACK=1 \
> > 
> > What happens when VIDEO_TRACK is enabled and HTML5 video or GStreamer isn't?
> 
> Now that you point this out, I'm confused about why video works at all in QtWebkit, since I can't figure out where VIDEO_ENABLED gets set to 1.

Tools/qmake/mkspecs/features/features.prf around line 120.
Comment 19 Brendan Long 2013-04-02 12:18:22 PDT
(In reply to comment #18)
> > Now that you point this out, I'm confused about why video works at all in QtWebkit, since I can't figure out where VIDEO_ENABLED gets set to 1.
> 
> Tools/qmake/mkspecs/features/features.prf around line 120.

The strange thing is that one line 120 my patch enables VIDEO_TRACK, but VIDEO is never enabled:

     ENABLE_VIBRATION=0 \
     ENABLE_VIDEO=0 \ <---
-    ENABLE_VIDEO_TRACK=0 \
+    ENABLE_VIDEO_TRACK=1 \
     ENABLE_VIEW_MODE_CSS_MEDIA=1 \
     ENABLE_WEBGL=0 \

I haven't messed with ENABLE_VIDEO, I just can't test turning it off if it's already off.
Comment 20 Allan Sandfeld Jensen 2013-04-02 12:37:15 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > > Now that you point this out, I'm confused about why video works at all in QtWebkit, since I can't figure out where VIDEO_ENABLED gets set to 1.
> > 
> > Tools/qmake/mkspecs/features/features.prf around line 120.
> 
> The strange thing is that one line 120 my patch enables VIDEO_TRACK, but VIDEO is never enabled:
> 
>      ENABLE_VIBRATION=0 \
>      ENABLE_VIDEO=0 \ <---
> -    ENABLE_VIDEO_TRACK=0 \
> +    ENABLE_VIDEO_TRACK=1 \
>      ENABLE_VIEW_MODE_CSS_MEDIA=1 \
>      ENABLE_WEBGL=0 \
> 
> I haven't messed with ENABLE_VIDEO, I just can't test turning it off if it's already off.

Those are just the defaults. Check features.prf (not features.pri which you are referring to).
Comment 21 Brendan Long 2013-04-02 14:26:28 PDT
Created attachment 196225 [details]
Patch
Comment 22 Brendan Long 2013-04-02 14:29:30 PDT
The new patch fixes a couple problems with the last one:

  * Removed the extra line from the change logs.
  * Enabled video_track in features.prf instead of features.pri. There's no point in having text tracks if you don't have media support, so this is cleaner.

I haven't changed anything about the tests because as far as I can tell, they're already enabled?
Comment 23 Brendan Long 2013-04-02 14:31:56 PDT
Created attachment 196226 [details]
Patch
Comment 24 Brendan Long 2013-04-02 14:33:29 PDT
(In reply to comment #23)
> Created an attachment (id=196226) [details]
> Patch

This just updates the changelog to today's date. I'm not sure if you care but I didn't want to take the risk that it would be rejected for that.
Comment 25 Allan Sandfeld Jensen 2013-04-03 02:53:49 PDT
(In reply to comment #22)
> The new patch fixes a couple problems with the last one:
> 
>   * Removed the extra line from the change logs.
>   * Enabled video_track in features.prf instead of features.pri. There's no point in having text tracks if you don't have media support, so this is cleaner.
> 
> I haven't changed anything about the tests because as far as I can tell, they're already enabled?

Many of them look like they already are enabled, but there are test blocking on bug #103769 and bug #103926.

But if they are already enabled I would like to know why they passed. Could you run them with and without your patch and check what happens?
Comment 26 Brendan Long 2013-04-03 08:08:45 PDT
(In reply to comment #25)
> Many of them look like they already are enabled, but there are test blocking on bug #103769 and bug #103926.

Bug #103769 is for in-band tracks. I was going to submit a patch for that one after this. Bug #103926 checks for some things that text tracks don't do yet, like the "remove" event, and specific positioning and WebVTT things. Those tests don't pass in any port, since that code isn't port-specific.

> But if they are already enabled I would like to know why they passed. Could you run them with and without your patch and check what happens?

Doing this now.
Comment 27 Brendan Long 2013-04-03 10:01:34 PDT
Created attachment 196367 [details]
Output of running run-webkit-tests LayoutTests/media/track/* without my patch

Interestingly, the page that opens up at the end lists "pass" in the "expected failure" column for all of the tests. Is there some place to make it not expect failure?
Comment 28 Brendan Long 2013-04-03 10:03:41 PDT
(In reply to comment #27)
> Created an attachment (id=196367) [details]
> Output of running run-webkit-tests LayoutTests/media/track/* without my patch
> 
> Interestingly, the page that opens up at the end lists "pass" in the "expected failure" column for all of the tests. Is there some place to make it not expect failure?

Oh, the git status output actually helped. It looks like I need to add a bunch of -expected.txt files in LayoutTests/platform/qt/media/track/
Comment 29 Brendan Long 2013-04-03 12:14:20 PDT
Created attachment 196386 [details]
Output of running run-webkit-tests LayoutTests/media/track/* with my patch

Here's the results after applying my patch.
Comment 30 Brendan Long 2013-04-03 12:15:54 PDT
Actually, with my patch, the tests that pass don't add anything to LayoutTests/platform/qt/media/track, so I still have no idea what's going on. You can see from the output that my patch does fix a bunch of tests though.
Comment 31 Brendan Long 2013-04-03 15:25:15 PDT
Is there anything else I need to do?
Comment 32 Brendan Long 2013-04-04 16:41:41 PDT
I just realized that there my be a better option than enabling this by default. If you commit all of the changes except features.prf, we can build with:

    Tools/Script/build-webkit --qt --video-track

I'm looking at adding this to build-webkit's --help output.

Would that be better?
Comment 33 Brendan Long 2013-04-04 16:59:05 PDT
(In reply to comment #32)
> I'm looking at adding this to build-webkit's --help output.

It's already there, I just didn't notice it apparently.
Comment 34 Jocelyn Turcotte 2013-04-05 02:31:39 PDT
(In reply to comment #32)
> I just realized that there my be a better option than enabling this by default. If you commit all of the changes except features.prf, we can build with:
> 
>     Tools/Script/build-webkit --qt --video-track
> 
> I'm looking at adding this to build-webkit's --help output.
> 
> Would that be better?

What we do is usually that the feature isn't enabled by default until it is production-ready. Developers of the feature can enabled it through build-webkit like you noticed until then.

All features should eventually be enabled by default as this is the way for production builds of QtWebKit to include the feature. So once you're satisfied with the quality of the code, we can verify and enable it by default.
Comment 35 Brendan Long 2013-04-05 07:45:29 PDT
(In reply to comment #34)
> What we do is usually that the feature isn't enabled by default until it is production-ready. Developers of the feature can enabled it through build-webkit like you noticed until then.

Since this feature isn't really finished, I think it's probably better right now to commit the changes to make the build work, but leave it disabled.

We probably want bug #103771 and #113965 (and whatever bug gets created to add audio/video track support to GStreamer) before enabling it by default.

Is there a way to tell the build bots to build with --video-track for a patch?
Comment 36 Jocelyn Turcotte 2013-04-05 07:50:48 PDT
(In reply to comment #35)
> Is there a way to tell the build bots to build with --video-track for a patch?

Build bots only need to verify that de default features aren't regressing.
If you are working on a feature, you are responsible for creating tests for it, running them and tracking the regressions yourself until it's ready.
Comment 37 Jocelyn Turcotte 2013-04-05 07:52:40 PDT
(In reply to comment #36)
But that said, if other ports have already enabled that feature by default, their bots should already test that feature so that your work isn't messing up with what's already there :)
Comment 38 Brendan Long 2013-04-05 09:24:30 PDT
Created attachment 196642 [details]
Patch
Comment 39 Brendan Long 2013-04-05 09:28:51 PDT
This patch makes the --video-track build work and doesn't enable it by default. I think that's probably what we want for now. I'll work on doing bug #103771, bug #113965 and whatever bug exists for audio and video tracks in GStreamer, and we can enable this by default whenever it works for you guys.
Comment 40 Jocelyn Turcotte 2013-04-05 09:32:10 PDT
Comment on attachment 196642 [details]
Patch

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

> Source/WebCore/Target.pri:690
> +    html/HTMLTrackElement.cpp \

I believe that all those should go in a "enable?(VIDEO_TRACK) { }" block, lower in this file.

Some file have #if guards for their features and some don't, so we compile feature-specific features only when enabled.
Comment 41 Brendan Long 2013-04-05 16:13:16 PDT
Created attachment 196695 [details]
Patch
Comment 42 Brendan Long 2013-04-05 16:15:43 PDT
(In reply to comment #40)
> (From update of attachment 196642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196642&action=review
> 
> > Source/WebCore/Target.pri:690
> > +    html/HTMLTrackElement.cpp \
> 
> I believe that all those should go in a "enable?(VIDEO_TRACK) { }" block, lower in this file.
> 
> Some file have #if guards for their features and some don't, so we compile feature-specific features only when enabled.

I moved all of the TextTrack and TextElement stuff into the ?enable(VIDEO_TRACK) block. I just finished rebuilding with and without --video-track and it works.
Comment 43 Brendan Long 2013-04-05 16:33:43 PDT
Created attachment 196697 [details]
Fix Target.pri where I mixed up HTMLTrackElement.cpp and .h
Comment 44 Jocelyn Turcotte 2013-04-08 02:32:18 PDT
Comment on attachment 196697 [details]
Fix Target.pri where I mixed up HTMLTrackElement.cpp and .h

The patch looks good to me.

If you want this patch officially reviewed please press "Details" on the attachment and set the "r" flag to "?". Please also set the "cq" flag to "?" if you want the patch to lanb now.
Comment 45 WebKit Commit Bot 2013-04-08 08:00:07 PDT
Comment on attachment 196697 [details]
Fix Target.pri where I mixed up HTMLTrackElement.cpp and .h

Rejecting attachment 196697 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 196697, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
2249 (offset -3 lines).
Hunk #7 succeeded at 3965 (offset -3 lines).
patching file Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp
Hunk #1 FAILED at 178.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp.rej
patching file Source/WebCore/platform/qt/LocalizedStringsQt.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Jocelyn Turcotte']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-commit-queue.appspot.com/results/17502346
Comment 46 Brendan Long 2013-04-08 08:06:51 PDT
Created attachment 196857 [details]
Patch
Comment 47 Brendan Long 2013-04-08 08:09:45 PDT
Rebased the patch so it will apply cleanly.
Comment 48 WebKit Commit Bot 2013-04-08 09:12:15 PDT
Comment on attachment 196857 [details]
Patch

Clearing flags on attachment: 196857

Committed r147917: <http://trac.webkit.org/changeset/147917>
Comment 49 WebKit Commit Bot 2013-04-08 09:12:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Allan Sandfeld Jensen 2013-08-14 09:28:36 PDT
(In reply to comment #49)
> All reviewed patches have been landed.  Closing bug.

Has the relevant tests been unskipped or tested? Can I assume this to work on Linux with GStreamer?
Comment 51 Brendan Long 2013-08-19 14:39:15 PDT
(In reply to comment #50)
> Has the relevant tests been unskipped or tested? Can I assume this to work on Linux with GStreamer?

Yes, I've been testing it with Linux + GStreamer. It shouldn't matter though, since this is out-of-band tracks.

I don't know how to unskip tests :\