Bug 113522 – [Qt] Enable text tracks from track elements

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Brendan Long]

[qt] Enable WebVTT text tracks and inband text tracks from GStreamer

[Brendan Long]

Created attachment 195604 [details]
Patch

[Brendan Long]

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.

[Brendan Long]

Also, should the component be WebKit Qt or Media Elements?

[Brendan Long]

It looks like I need to rebase my patch. I started a fetch and will reupload the patch after lunch.

[Alexis Menard (darktears)]

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.

[Alexis Menard (darktears)]

(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.

[Brendan Long]

Created attachment 195660 [details]
Patch

[Brendan Long]

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..

[Brendan Long]

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..

[Brendan Long]

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.

[Brendan Long]

Created attachment 196000 [details]
Patch

[Allan Sandfeld Jensen]

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?

[Philippe Normand]

(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?

[Brendan Long]

(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).

[Philippe Normand]

I suppose it can be closed as a dupe of bug 103771

[Brendan Long]

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?

[Brendan Long]

(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.

[Alexis Menard (darktears)]

(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.

[Brendan Long]

(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.

[Allan Sandfeld Jensen]

(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).

[Brendan Long]

Created attachment 196225 [details]
Patch

[Brendan Long]

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?

[Brendan Long]

Created attachment 196226 [details]
Patch

[Brendan Long]

(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.

[Allan Sandfeld Jensen]

(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?

[Brendan Long]

(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.

[Brendan Long]

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?

[Brendan Long]

(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/

[Brendan Long]

Created attachment 196386 [details]
Output of running run-webkit-tests LayoutTests/media/track/* with my patch

Here's the results after applying my patch.

[Brendan Long]

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.

[Brendan Long]

Is there anything else I need to do?

[Brendan Long]

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?

[Brendan Long]

(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.

[Jocelyn Turcotte]

(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.

[Brendan Long]

(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?

[Jocelyn Turcotte]

(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.

[Jocelyn Turcotte]

(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 :)

[Brendan Long]

Created attachment 196642 [details]
Patch

[Brendan Long]

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.

[Jocelyn Turcotte]

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.

[Brendan Long]

Created attachment 196695 [details]
Patch

[Brendan Long]

(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.

[Brendan Long]

Created attachment 196697 [details]
Fix Target.pri where I mixed up HTMLTrackElement.cpp and .h

[Jocelyn Turcotte]

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.

[WebKit Commit Bot]

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

[Brendan Long]

Created attachment 196857 [details]
Patch

[Brendan Long]

Rebased the patch so it will apply cleanly.

[WebKit Commit Bot]

Comment on attachment 196857 [details]
Patch

Clearing flags on attachment: 196857

Committed r147917: <http://trac.webkit.org/changeset/147917>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

(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?

[Brendan Long]

(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 :\