Bug 103771 – [GStreamer] support in-band text tracks

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

In-band caption track support requires changes in the media engine.

[Philippe Normand]

*** Bug 109378 has been marked as a duplicate of this bug. ***

[Brendan Long]

I'm working on this, plus audio and video tracks. The HTML5 spec requires that we mix all enabled audio tracks, and display all showing text tracks. The problem is that playbin can only play one track of each type at a time. The solution seems to be to build a pipeline using uridecodebin, but it's a pretty substantial change so I want to make sure that's acceptable before I do it.

Does anyone have a problem with me replacing playbin with uridecodebin to make multiple tracks work?

[Brendan Long]

Also, in GStreamer 1.0, the Kate subtitle decoder hasn't been ported (https://bugzilla.gnome.org/show_bug.cgi?id=697071), so I can't use .ogv files in tests. The only subtitles I've got working on GStreamer 1.0 are MKV files decoded by FFMPEG. Would that be ok?

[Eric Carlson]

(In reply to comment #2)
> I'm working on this, plus audio and video tracks. The HTML5 spec requires that we mix all enabled audio tracks, and display all showing text tracks. The problem is that playbin can only play one track of each type at a time. 
> 
For whatever it is worth, AVFoundation has this same restriction.

[Philippe Normand]

Adding Sebastian in CC, we were talking about this topic few days ago.

Right now I don't know what to do about this audio track mixing, seems like we reach a limitation of playbin :(

[Brendan Long]

(In reply to comment #5)
> Adding Sebastian in CC, we were talking about this topic few days ago.
> 
> Right now I don't know what to do about this audio track mixing, seems like we reach a limitation of playbin :(

We don't have to use playbin though. uridecodebin will give us all of the individual streams, it's just a little bit more complicated and I want to make sure it would be an acceptable change.

  * For the video we can just take the selected video stream and send it to an autovideosink.
  * For the audio we can take all enabled audio streams and send them to an adder connected to an autoaudiosink.
  * For the text, we just attach appsinks to each enabled or hidden stream.

[Sebastian Dröge (slomo)]

Not using playbin will give you more flexibility but you'll have to re-implement most of what playbin does on top of uridecodebin (don't underestimate that).

That would be one possibility to implement these features, the other would be to integrate everything needed into playbin directly. I think the latter should be the goal in the long run at least.

[Philippe Normand]

Brendan, for audio/video tracks support please refer to bug 113514. Bug 103771 is about text tracks

[Brendan Long]

(In reply to comment #7)
> That would be one possibility to implement these features, the other would be to integrate everything needed into playbin directly. I think the latter should be the goal in the long run at least.

I'm not sure that's possible without significant changes to playbin's API, and I don't think the GStreamer developers are in a big hurry to break their API's again.

From what I can see, it doesn't look like there's much we could add to playbin anyway. It would be nice if playbin did the audio mixing for us, but for text we need individual pads, which is what decodebin gives us.

[Sebastian Dröge (slomo)]

(In reply to comment #9)
> (In reply to comment #7)
> > That would be one possibility to implement these features, the other would be to integrate everything needed into playbin directly. I think the latter should be the goal in the long run at least.
> 
> I'm not sure that's possible without significant changes to playbin's API, and I don't think the GStreamer developers are in a big hurry to break their API's again.

Well, I'm one of those GStreamer developers and I think it's a good idea to add support for that in playbin... if necessary create another playbin2 for this :)

> From what I can see, it doesn't look like there's much we could add to playbin anyway. It would be nice if playbin did the audio mixing for us, but for text we need individual pads, which is what decodebin gives us.

There could be for audio/video/text a choice between:
- Allowing a sink per stream (-> gives you what you want for text)
- Allowing a single sink and then let GStreamer do some mixing which can be controlled via properties (-> gives you what you want for audio, and probably also for video)

[Brendan Long]

I'll start with a patch that uses playbin to allow one text output (using an appsink).

I'll also grab playbin and try to figure out what we'll need to do to make it handle multiple streams.

[Brendan Long]

By the way, I've been trying to port Kate subtitles to GStreamer 1.0 so we can use .ogv files in the layout tests. If anyone who understands GStreamer better could take a look, I'd appreciate it:

https://bugzilla.gnome.org/show_bug.cgi?id=697071

[Brendan Long]

I'm about to upload a patch to uses uridecodebin and playsink to implement this. I tried using playbin, but I kept coming back to how uridecodebin is a perfect match for what we're doing (handling streams individually).

One thing that would be nice is if playsink had multiple inputs and outputs and had:

  * An adder on the audio streams (with some way to enable and disable them).
  * An inputselector on the video streams

The changes I made are:

  * m_playbin becomes m_pipeline, which has a uridecodebin and a playsink
  * We automatically connect the first audio and video stream to the playsink
  * We connect all text streams to appsinks
  * Add InbandTextTrackPrivateGStreamer
  * InbandTextTrackPrivateClient gets setLabel() and setLanguage()
  * HTMLMediaElement::updateActiveTextTrackCues() ignores disabled tracks
  * Made GRefPtr<GstTagList> work
  * Simplify position query to just use gst_element_query_position()
  * Add test track-in-band-no-forced.html and update track-in-band-cues-added-once.html to work with this implementation (You'll need gst-plugins-bad from git to run these)

Some outstanding issues (in progress):

  * On GStreamer 0.10, restarting the video after it ends doesn't work.
  * When two cues are displayed at the same time, they get rendered on top of eachother.
  * I'm probably not handling all of the GRefPtr's correctly at the moment.
  * Katedec doesn't seem to be giving me any tags right now on 1.0

It's obviously not done, but I wanted to get this out here to see if I'm doing anything obviously wrong at the moment. It should build on Qt with GStreamer 0.10 and 1.0, and hopefully GTK. Do any other ports use GStreamer?

I'll add the audio and video tracks next.

[Martin Robinson]

(In reply to comment #13)

> It's obviously not done, but I wanted to get this out here to see if I'm doing anything obviously wrong at the moment. It should build on Qt with GStreamer 0.10 and 1.0, and hopefully GTK. Do any other ports use GStreamer?

Sorry in advance, if this comment is misguided. I think that any solution that doesn't use WebCore ResourceHandle or gets WebKit further from using ResourceHandle for video is probably the wrong one.

[Brendan Long]

(In reply to comment #14)
> Sorry in advance, if this comment is misguided. I think that any solution that doesn't use WebCore ResourceHandle or gets WebKit further from using ResourceHandle for video is probably the wrong one.

What's ResourceHandle?

[Martin Robinson]

(In reply to comment #15)
> (In reply to comment #14)
> > Sorry in advance, if this comment is misguided. I think that any solution that doesn't use WebCore ResourceHandle or gets WebKit further from using ResourceHandle for video is probably the wrong one.
> 
> What's ResourceHandle?

ResourceHandle is the networking layer in WebCore. For GTK+/EFL this is in platform/network/soup and for qt it's in platform/network/qt. The point being that networking should be done by WebCore and not by gstreamer. If your patch doesn't change that or make it harder, then I'll defer to GStreamer experts to comment. :)

[Brendan Long]

Created attachment 199529 [details]
uridecodebin+playsink

[Brendan Long]

I don't think this patch makes it any harder. Currently, we're using playbin (presumably with a uridecodebin internally) to handle networking. My patch will make us use uridecodebin to handle networking, and playsink to handle everything else.

To use ResourceLoader, I assume we'd have to use an appsrc + decodebin instead, but I think that's outside the scope of this change.

[Build Bot]

Comment on attachment 199529 [details]
uridecodebin+playsink

Attachment 199529 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/186606

New failing tests:
media/track/track-in-band-no-forced.html

[Build Bot]

Created attachment 199532 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[EFL EWS Bot]

Comment on attachment 199529 [details]
uridecodebin+playsink

Attachment 199529 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/26778

[EFL EWS Bot]

Comment on attachment 199529 [details]
uridecodebin+playsink

Attachment 199529 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/114482

[Sebastian Dröge (slomo)]

(In reply to comment #13)
> I'm about to upload a patch to uses uridecodebin and playsink to implement this. I tried using playbin, but I kept coming back to how uridecodebin is a perfect match for what we're doing (handling streams individually).
> 
> One thing that would be nice is if playsink had multiple inputs and outputs and had:
> 
>   * An adder on the audio streams (with some way to enable and disable them).
>   * An inputselector on the video streams

What you describe here is exactly what should be inside playbin and exposed via some new API on playbin (and the second item is actually implemented already, and not only for video but audio and text too). playbin is using uridecodebin and playsink internally, and lots of glue to make them both work perfectly together. The latter is what would have to be reimplemented if you stop using playbin.

The selector and mixing part of streams of the same type (i.e. using inputselector or using adder/videomixer) should be done between uridecodebin and playsink.

[Sebastian Dröge (slomo)]

(In reply to comment #13)

> It's obviously not done, but I wanted to get this out here to see if I'm doing anything obviously wrong at the moment. It should build on Qt with GStreamer 0.10 and 1.0, and hopefully GTK. Do any other ports use GStreamer?

The Qt port is using GStreamer too, IIRC there's also a Windows/cairo port that can use GStreamer and externally there's a clutter port. Probably more

[Philippe Normand]

(In reply to comment #24)
> (In reply to comment #13)
> 
> > It's obviously not done, but I wanted to get this out here to see if I'm doing anything obviously wrong at the moment. It should build on Qt with GStreamer 0.10 and 1.0, and hopefully GTK. Do any other ports use GStreamer?
> 
> The Qt port is using GStreamer too, IIRC there's also a Windows/cairo port that can use GStreamer and externally there's a clutter port. Probably more

Right now, upstream, the ports using GStreamer are: GTK+, Qt, EFL.

[Sebastian Dröge (slomo)]

(In reply to comment #23)
> (In reply to comment #13)
> > I'm about to upload a patch to uses uridecodebin and playsink to implement this. I tried using playbin, but I kept coming back to how uridecodebin is a perfect match for what we're doing (handling streams individually).
> > 
> > One thing that would be nice is if playsink had multiple inputs and outputs and had:
> > 
> >   * An adder on the audio streams (with some way to enable and disable them).
> >   * An inputselector on the video streams
> 
> What you describe here is exactly what should be inside playbin and exposed via some new API on playbin (and the second item is actually implemented already, and not only for video but audio and text too). playbin is using uridecodebin and playsink internally, and lots of glue to make them both work perfectly together. The latter is what would have to be reimplemented if you stop using playbin.
> 
> The selector and mixing part of streams of the same type (i.e. using inputselector or using adder/videomixer) should be done between uridecodebin and playsink.

The best way to move this forward would be a bug at http://bugzilla.gnome.org against GStreamer. Some ideas for the implementation would be 1) allowing the application to set a N->1 "stream combiner" element on playbin for the A/V/T streams, or 2) some API to select between selection/mixing and allow to set different mixing options (volume per audio stream, video stream positions/scaling, ...)

[Brendan Long]

(In reply to comment #23)
> What you describe here is exactly what should be inside playbin and exposed via some new API on playbin (and the second item is actually implemented already, and not only for video but audio and text too).

The thing that makes me want to not use playbin is that we need to handle every stream differently, and that seems to not mesh well with using playbin. For video, we want an inputselector, which playbin provides, but for audio we want an adder, and for text we just want all of the pads exposed. It seems like playbin is a one-size-fits-all solution, and that's not really what we need.

> playbin is using uridecodebin and playsink internally, and lots of glue to make them both work perfectly together. The latter is what would have to be reimplemented if you stop using playbin.

I haven't got to video yet, but don't I just need to take any pad I see in pad-added and add it to the video inputselector? The glue I have so far is just handling padAdded() and connecting audio and video to a playsink, and connecting text to a queue then appsink. Ignoring callbacks and other things that I would need anyway, it's only about 40 lines of code, and using uridecodebin makes other parts of the code simpler, since pad-added and pad-removed are easier to work with than text-changed.

[Brendan Long]

One more question: Is there a way to figure out the track "kind" in GStreamer? Should there be a GST_TAG_KIND?

[Sebastian Dröge (slomo)]

(In reply to comment #27)
> (In reply to comment #23)
> > What you describe here is exactly what should be inside playbin and exposed via some new API on playbin (and the second item is actually implemented already, and not only for video but audio and text too).
> 
> The thing that makes me want to not use playbin is that we need to handle every stream differently, and that seems to not mesh well with using playbin. For video, we want an inputselector, which playbin provides, but for audio we want an adder, and for text we just want all of the pads exposed. It seems like playbin is a one-size-fits-all solution, and that's not really what we need.

If you were able to set a N->1 stream combiner element for audio, video and text you would be able to handle all these cases just fine in playbin without losing the comfort of all the other playbin features.

> > playbin is using uridecodebin and playsink internally, and lots of glue to make them both work perfectly together. The latter is what would have to be reimplemented if you stop using playbin.
> 
> I haven't got to video yet, but don't I just need to take any pad I see in pad-added and add it to the video inputselector? The glue I have so far is just handling padAdded() and connecting audio and video to a playsink, and connecting text to a queue then appsink. Ignoring callbacks and other things that I would need anyway, it's only about 40 lines of code, and using uridecodebin makes other parts of the code simpler, since pad-added and pad-removed are easier to work with than text-changed.

Take a look at the playbin code and you'll see that there's much more involved :)

(In reply to comment #28)
> One more question: Is there a way to figure out the track "kind" in GStreamer? Should there be a GST_TAG_KIND?

What do you mean with kind? If it's audio/video/text? Look at gstplaybin2.c, the code that checks to which input-selector a pad of uridecodebin belongs.

[Brendan Long]

(In reply to comment #29)
> What do you mean with kind? If it's audio/video/text? Look at gstplaybin2.c, the code that checks to which input-selector a pad of uridecodebin belongs.

The "kind" attribute is supposed to say if something is subtitles, captions, descriptions, chapters or metadata:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#text-track-kind

There's something similar for audio and video tracks:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-audiotrack-kind

[Sebastian Dröge (slomo)]

(In reply to comment #30)
> (In reply to comment #29)
> > What do you mean with kind? If it's audio/video/text? Look at gstplaybin2.c, the code that checks to which input-selector a pad of uridecodebin belongs.
> 
> The "kind" attribute is supposed to say if something is subtitles, captions, descriptions, chapters or metadata:
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#text-track-kind
> 
> There's something similar for audio and video tracks:
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-audiotrack-kind

No, I don't think that this exists yet. Would be useful to add some new tags for that though

[Brendan Long]

I created this bug against GStreamer: https://bugzilla.gnome.org/show_bug.cgi?id=698851

[Brendan Long]

And this for the "kind" attribute: https://bugzilla.gnome.org/show_bug.cgi?id=698853

[Brendan Long]

Also, it's not clear to me if GST_TAG_TITLE should be used as the "label" attribute. I'm not sure if there's anything better to use though.

[Brendan Long]

Created attachment 199670 [details]
EFL support

[Brendan Long]

In case anyone else finds this patch useful, this should add EFL support. I need something working relatively soon, so I'll be finishing this patch and possibly looking into the playbin stuff if I have time.

[EFL EWS Bot]

Comment on attachment 199670 [details]
EFL support

Attachment 199670 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/64236

[EFL EWS Bot]

Comment on attachment 199670 [details]
EFL support

Attachment 199670 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/171307

[Sebastian Dröge (slomo)]

(In reply to comment #34)
> Also, it's not clear to me if GST_TAG_TITLE should be used as the "label" attribute. I'm not sure if there's anything better to use though.

Title sounds good to me

[Brendan Long]

Created attachment 199675 [details]
Fix assert misuse and unused variable warnings

[EFL EWS Bot]

Comment on attachment 199675 [details]
Fix assert misuse and unused variable warnings

Attachment 199675 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/231045

[EFL EWS Bot]

Comment on attachment 199675 [details]
Fix assert misuse and unused variable warnings

Attachment 199675 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/49645

[Build Bot]

Comment on attachment 199675 [details]
Fix assert misuse and unused variable warnings

Attachment 199675 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/26898

New failing tests:
media/track/track-in-band-no-forced.html

[Build Bot]

Created attachment 199692 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Brendan Long]

I have a question about the format for cues. It looks like currently, GStreamer subtitle plugins generate text with Pango markup. We could just parse that in WebKit and generate cues, but it seems like it may be easier to have GStreamer output WebVTT. I think we could do that by setting caps on our text appsink for text/vtt, and presumably vttenc would be pulled in if necessary. Then we could probably use the same parser the out-of-band tracks use. Does doing that make sense?

I'm currently having a discussion with coworkers about what be most likely to be accepted, so I figured it would be easier to just ask.

[Philippe Normand]

(In reply to comment #45)
> I have a question about the format for cues. It looks like currently, GStreamer subtitle plugins generate text with Pango markup. We could just parse that in WebKit and generate cues, but it seems like it may be easier to have GStreamer output WebVTT. I think we could do that by setting caps on our text appsink for text/vtt, and presumably vttenc would be pulled in if necessary. Then we could probably use the same parser the out-of-band tracks use. Does doing that make sense?
> 

It makes sense I think. However I'm not sure how webvttenc would be "pulled in automatically". I need to read your patch again but it's quite big :)

[Brendan Long]

(In reply to comment #46)
> It makes sense I think. However I'm not sure how webvttenc would be "pulled in automatically".

If we set the caps (on the appsink?) to text/vtt, wouldn't decodebin autoplug the webvttenc? I don't really know much about autoplugging, but I assume it would see that we have text/x-raw coming in and want text/vtt going out, and plug in the webvttenc to do the conversion.

> I need to read your patch again but it's quite big :)

I'm still planning to just add the stream combiner thing to playbin, I've just been really busy recently. I actually have a full patch with audio and video tracks too if anyone is interested, but it has some adder-related problems.

[Philippe Normand]

Hi Brendan,

So now that you extended playbin I suppose this patch can be revamped? :)

[Brendan Long]

(In reply to comment #48)
> So now that you extended playbin I suppose this patch can be revamped? :)

Already working on it. I'm actually planning to submit the audio+video part first (I'll open another bug and link to it), since it's a lot simpler.

[Brendan Long]

I added bug #117039 to add audio and video tracks. I decided to do that first because it's almost exactly the same, but simpler because I don't need to handle cues.

[Brendan Long]

I'm working on an element to combine the text streams: https://bugzilla.gnome.org/show_bug.cgi?id=703267

[Brendan Long]

Created attachment 206687 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:42:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:181:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 18 files


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

[EFL EWS Bot]

Comment on attachment 206687 [details]
Patch

Attachment 206687 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1071578

[EFL EWS Bot]

Comment on attachment 206687 [details]
Patch

Attachment 206687 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1069620

[kov's GTK+ EWS bot]

Comment on attachment 206687 [details]
Patch

Attachment 206687 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1069622

[Brendan Long]

Here's a new patch that uses playbin.

I need two WebVTTParser patches to make this work:

  * Bug #118483 - WebVTTParser's identifier buffering can ignore subsequent lines
  * Bug #118687 - Make WebVTTParser return cue data instead of cue DOM objects

This only works with GStreamer >= 1.1.2, because we need the "text-stream-combiner" property and we need funnel to work in playbin. I do the checks at runtime though, so InbandTextTrackPrivateGStreamer and TextCombinerGStreamer are only built for GStreamer >= 1.0, and we only setup the stream combiner and callbacks when running against GStreamer >= 1.1.2.

TextCombinerGStreamer is a bin with a funnel. It adds "webvttenc" elements as needed to get WebVTT output (based on caps events). I initially tried to use 
"autoconvert", but it wouldn't link.

We use stream start events to determine which stream a cue belongs to, and watch for tag events to get the label and language (still need to add TAG_KIND for the "kind" attribute). I added labelChanged and languageChanged callbacks to InbandTextTrack.

This patch won't pass style because GLib's macros name init functions with underscores. That's the only style problem though.

It still needs some updates to the tests, and I'm having a problem where videos with multiple text tracks get cues slowly sometimes (I think stream synchronizer may be holding on to them too long?).

[Philippe Normand]

Comment on attachment 206687 [details]
Patch

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

Haven't done a full review yet, just a question:

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1728
> +    if (webkitGstCheckVersion(1, 1, 2)) {

Why not using the GST_CHECK_VERSION macro? Doing the test at build time would allow EWS to pass I think. We have no EWS with gst 1.1.2 yet though. Perhaps it's too early to bump the dep in our jhbuild moduleset too.

[Brendan Long]

(In reply to comment #58)
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1728
> > +    if (webkitGstCheckVersion(1, 1, 2)) {
> 
> Why not using the GST_CHECK_VERSION macro? Doing the test at build time would allow EWS to pass I think. We have no EWS with gst 1.1.2 yet though. Perhaps it's too early to bump the dep in our jhbuild moduleset too.

The EWS builds are failing because they need the WebVTTParser patch. For building, all that matters is that we're on GStreamer 1.0.

The runtime check is because before that version, the API is fine, but we'll get crashes and lockups, so I wanted things to work in the case where someone builds against 1.1.2 and then runs against 1.1.1. I guess that's not common, but that was the idea.

[Brendan Long]

Created attachment 206816 [details]
Patch

[Brendan Long]

Now it should build..

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:42:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:181:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 17 files


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

[EFL EWS Bot]

Comment on attachment 206816 [details]
Patch

Attachment 206816 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1114064

[EFL EWS Bot]

Comment on attachment 206816 [details]
Patch

Attachment 206816 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1091301

[kov's GTK+ EWS bot]

Comment on attachment 206816 [details]
Patch

Attachment 206816 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1091303

[Brendan Long]

Created attachment 206818 [details]
Fix build warnings

[Brendan Long]

Is there a way to make QtWebKit build with the same warnings / errors as WebKitGTK and EWebKit?

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:42:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:181:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 17 files


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

[kov's GTK+ EWS bot]

Comment on attachment 206818 [details]
Fix build warnings

Attachment 206818 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1083320

[Brendan Long]

Created attachment 206820 [details]
Fix textTrackIndex() function

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:42:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:181:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 17 files


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

[kov's GTK+ EWS bot]

Comment on attachment 206820 [details]
Fix textTrackIndex() function

Attachment 206820 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1115001

[Brendan Long]

> In file included from ../../Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:24:0:
../../Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:27:26: fatal error: WebVTTParser.h: No such file or directory
compilation terminated.

WebVTTParser is in GNUmakefile.list.am. Is the problem that it's in webcore_sources instead of platform_sources?

[Philippe Normand]

Comment on attachment 206820 [details]
Fix textTrackIndex() function

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

> Source/WebCore/GNUmakefile.list.am:6375
> +	Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp \
> +	Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h \

These 2 at least need to be in webcore_platform_sources, not in platform_sources.
I suspect the same is needed for TextCombinerGStreamer files.

[Brendan Long]

Created attachment 206900 [details]
Put InbandTextTrackPrivateGStreamer in webcore_platform_sources

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:42:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:181:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 17 files


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

[Brendan Long]

(In reply to comment #74)
> These 2 at least need to be in webcore_platform_sources, not in platform_sources.
> I suspect the same is needed for TextCombinerGStreamer files.

Thanks. TextCombinerGStreamer doesn't have any WebKit dependencies, so I left it alone.

[Brendan Long]

I seem to remember when I first started on this that GStreamer didn't like the counting-subtitled.m4v file, but it seems to work now, except that qtdemux only finds the first text stream, and there's no way to determine if the subtitles are "forced".

[Brendan Long]

For the qtdemux thing, it looks like it was completely ignoring disabled streams, and like usual.. someone else just fixed it.

[Brendan Long]

But the problem with some cues not showing up is significantly worse with the counting-subtitled.m4v video. I only get one or two cues and then nothing. If I use a gst-plugins-good commit before the disabled streams fix, I get all of the cues for one stream perfectly. Like before, I'm guessing this is related to streamsynchronizer. I'll look into it more.

[Brendan Long]

I filed a GStreamer bug for the cues not showing up correctly when there's more than one stream going through the funnel. Further testing suggests that it's a problem with "queue", not "streamsynchronizer". I'll continue investigating tomorrow.

https://bugzilla.gnome.org/show_bug.cgi?id=704427

[Philippe Normand]

(In reply to comment #78)
> I seem to remember when I first started on this that GStreamer didn't like the counting-subtitled.m4v file, but it seems to work now, except that qtdemux only finds the first text stream, and there's no way to determine if the subtitles are "forced".

I think I filed a gst bug for this some years ago :) Will try to find it

[Philippe Normand]

(In reply to comment #82)
> (In reply to comment #78)
> > I seem to remember when I first started on this that GStreamer didn't like the counting-subtitled.m4v file, but it seems to work now, except that qtdemux only finds the first text stream, and there's no way to determine if the subtitles are "forced".
> 
> I think I filed a gst bug for this some years ago :) Will try to find it

https://bugzilla.gnome.org/show_bug.cgi?id=606643 but it's about counting-captioned.mov

[Brendan Long]

(In reply to comment #81)
> I filed a GStreamer bug for the cues not showing up correctly when there's more than one stream going through the funnel. Further testing suggests that it's a problem with "queue", not "streamsynchronizer". I'll continue investigating tomorrow.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=704427

Sebastian Dröge fixed this this morning.

I'm looking into another problem where cues sometimes show up late. I also found a problem with the "counting-subtitled.m4v" file where some of the French subtitles are in English, which I thought was a bug in my code until I realized that VLC did it too.

[Brendan Long]

(In reply to comment #84)
> I'm looking into another problem where cues sometimes show up late.

I think this problem is because we can get a situation like this:

0.0 --> 1.0
Track 1, Cue 1

1.0 --> 2.0
Track 1, Cue 2

0.0 --> 1.0
Track 2, Cue 2

We don't see "Track 2, Cue 2" until after "Track 1, Cue 2", but by that point, it's already too late to display it. Setting appsink sink=false didn't help, and neither did adding a streamsynchronizer before the funnel..

[Sebastian Dröge (slomo)]

(In reply to comment #85)
> (In reply to comment #84)
> > I'm looking into another problem where cues sometimes show up late.
> 
> I think this problem is because we can get a situation like this:
> 
> 0.0 --> 1.0
> Track 1, Cue 1
> 
> 1.0 --> 2.0
> Track 1, Cue 2
> 
> 0.0 --> 1.0
> Track 2, Cue 2
> 
> We don't see "Track 2, Cue 2" until after "Track 1, Cue 2", but by that point, it's already too late to display it. Setting appsink sink=false didn't help, and neither did adding a streamsynchronizer before the funnel..

Could you get a complete pipeline dump after everything is set up?
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstInfo.html#GST-DEBUG-BIN-TO-DOT-FILE:CAPS

I'm not 100% sure which element is involved here with synchronizing (wrongly) the different subtitle streams.

[Brendan Long]

Created attachment 207336 [details]
Pipeline while playing

(In reply to comment #86)
> Could you get a complete pipeline dump after everything is set up?
> 
> I'm not 100% sure which element is involved here with synchronizing (wrongly) the different subtitle streams.

[Sebastian Dröge (slomo)]

(In reply to comment #87)
> Created an attachment (id=207336) [details]
> Pipeline while playing
> 
> (In reply to comment #86)
> > Could you get a complete pipeline dump after everything is set up?
> > 
> > I'm not 100% sure which element is involved here with synchronizing (wrongly) the different subtitle streams.

Nothing there is synchronizing anything in the text chain, maybe exactly that is the problem :) You might want to add some kind of synchronizing element before the funnel in your text chain that synchronizes both text streams to each other. Or maybe that could be added to funnel as an optional feature.

[Brendan Long]

(In reply to comment #88)
> Nothing there is synchronizing anything in the text chain, maybe exactly that is the problem :)

What about the streamsynchronizer in playsink (streamsynchronizer0 in this pipeline)? It synchronizes the text stream with the video stream, so in my example (the cues from 0->1, 1->2, 0->1), wouldn't it wait for the video to catch up before passing on the 1->2 cue?

> You might want to add some kind of synchronizing element before the funnel in your text chain that synchronizes both text streams to each other. Or maybe that could be added to funnel as an optional feature.

Not being synchronized would be fine, because I don't care about the order of buffers, and getting them sooner would actually be good since it would give me more time to parse them.

I actually tried adding a streamsynchronizer before the funnel and it didn't seem to change anything.

[Sebastian Dröge (slomo)]

(In reply to comment #89)
> (In reply to comment #88)
> > Nothing there is synchronizing anything in the text chain, maybe exactly that is the problem :)
> 
> What about the streamsynchronizer in playsink (streamsynchronizer0 in this pipeline)? It synchronizes the text stream with the video stream, so in my example (the cues from 0->1, 1->2, 0->1), wouldn't it wait for the video to catch up before passing on the 1->2 cue?

streamsynchronizer is only aligning the segments and EOS of streams, it's not synchronizing any streams during the runtimes. That's the job of the sinks.

> > You might want to add some kind of synchronizing element before the funnel in your text chain that synchronizes both text streams to each other. Or maybe that could be added to funnel as an optional feature.
> 
> Not being synchronized would be fine, because I don't care about the order of buffers, and getting them sooner would actually be good since it would give me more time to parse them.

What's the problem then? :)

[Brendan Long]

(In reply to comment #90)
> What's the problem then? :)

The problem is that *something* is preventing me from getting the cues on time. I'll look into it more though. Knowing that the syncs are doing the synchronization is really helpful.

[Sebastian Dröge (slomo)]

(In reply to comment #91)
> (In reply to comment #90)
> > What's the problem then? :)
> 
> The problem is that *something* is preventing me from getting the cues on time. I'll look into it more though. Knowing that the syncs are doing the synchronization is really helpful.

Yeah, try sync=false on the appsink :)

[Sebastian Dröge (slomo)]

(In reply to comment #92)
> (In reply to comment #91)
> > (In reply to comment #90)
> > > What's the problem then? :)
> > 
> > The problem is that *something* is preventing me from getting the cues on time. I'll look into it more though. Knowing that the syncs are doing the synchronization is really helpful.
> 
> Yeah, try sync=false on the appsink :)

Then you only need to do synchronization yourself obviously, but that you have to do anyway because of having the multiple subtitle streams in one.

[Brendan Long]

Created attachment 207418 [details]
Output from adding a probe after the webvttenc and before the funnel.

I added probes before and after the funnel to see if something is going on in there, and I got some interesting output. It looks like part of the problem is that something is slow, so we get a cue exactly when we're supposed to:

Got data in at 0:00:12.149604166: 00:00:12.149 --> 00:00:14.977
Cosa ti porta nelle
terre dei guardiani?

But it leaves the funnel half a second late:

Got data out at 0:00:12.513583333: 00:00:12.149 --> 00:00:14.977
Cosa ti porta nelle
terre dei guardiani?

The other problem is that some of them come in *really* late:

Got data in at 0:00:18.188312500: 00:00:12.149 --> 00:00:14.977
Was treibt dich ins
Land der Torwächter?

Which is suspiciously just in time for the next cue:

Got data in at 0:00:18.189270833: 00:00:18.563 --> 00:00:20.087
למישהו מחפסת אני

(Video is Sintel: http://ftp.nluug.nl/ftp/graphics/blender/apricot/trailer/Sintel_Trailer1.480p.DivX_Plus_HD.mkv)

I'll keep investigating to see if the thread is being held up or something.

[Sebastian Dröge (slomo)]

(In reply to comment #94)
> Created an attachment (id=207418) [details]
> Output from adding a probe after the webvttenc and before the funnel.
> 
> I added probes before and after the funnel to see if something is going on in there, and I got some interesting output. It looks like part of the problem is that something is slow, so we get a cue exactly when we're supposed to:

Did you set sync=false on the appsink? If you didn't, from a short look this looks like the expected behaviour then

[Brendan Long]

(In reply to comment #95)
> Did you set sync=false on the appsink?

Yes:

        g_object_set(m_textAppSink.get(), "emit-signals", true, "enable-last-sample", false,
            "caps", gst_caps_new_empty_simple("text/vtt"), "sync", false, NULL);

[Brendan Long]

Created attachment 207469 [details]
Unref pad after usage, use static casts, change license on new files to BSD

I updated the patch based on comments on the audio/video track bug. Also, my company wanted the license on the new files to be BSD. I assume it doesn't make any difference, since MediaPlayerPrivateGStreamer is LGPL, but legal gets what they want..

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 17 files


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

[Sebastian Dröge (slomo)]

(In reply to comment #96)
> (In reply to comment #95)
> > Did you set sync=false on the appsink?
> 
> Yes:
> 
>         g_object_set(m_textAppSink.get(), "emit-signals", true, "enable-last-sample", false,
>             "caps", gst_caps_new_empty_simple("text/vtt"), "sync", false, NULL);

Do you have a gstreamer-only testcase for this to reproduce it? Something without all the 1000 layers of webkit involved? :)

[Brendan Long]

(In reply to comment #99)
> Do you have a gstreamer-only testcase for this to reproduce it? Something without all the 1000 layers of webkit involved? :)

I created a GStreamer bug with a Python test case: https://bugzilla.gnome.org/show_bug.cgi?id=704946

All it does is set a funnel as the text-stream-combiner, hook up and app-sink, and print the current time, cue start time, cue end time, and if the cue is so late it can't be displayed.

[Brendan Long]

Created attachment 207755 [details]
Extend appsink to prevent playsink from setting sync=true.

I created another class, WebKitTextSink, which ignores the 'sync' property and position and duration queries. The sync property needs to be ignored so playsink won't override it. The duration and position need to be ignored so the appsink isn't used to determine the seek bar's position.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:43:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:77:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 19 files


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

[Brendan Long]

Created attachment 207761 [details]
Fix properties to start at 0

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 19 files


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

[Brendan Long]

Created attachment 207777 [details]
Fix some more leaks with adoptGRef()

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 19 files


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

[Philippe Normand]

Comment on attachment 207777 [details]
Fix some more leaks with adoptGRef()

I haven't had time to review this yet but I have one question :) Would this patch fix some of the media/track tests? At least media/track/track-in-band.html ? It'd be great this is patch help unskip layout tests.

[Brendan Long]

(In reply to comment #107)
> (From update of attachment 207777 [details])
> I haven't had time to review this yet but I have one question :) Would this patch fix some of the media/track tests? At least media/track/track-in-band.html ? It'd be great this is patch help unskip layout tests.

track-in-band.html expects the "kind" to be set to captions, but there's not currently any way to figure out the track kind in GStreamer (see: https://bugzilla.gnome.org/show_bug.cgi?id=698853). Also, it expects "forced" tracks, but I can't find them in the HTML5 spec.

I was thinking of creating a copy of counting-subtitled.m4v without forced subtitles and using that for new tests..

[Eric Carlson]

(In reply to comment #108)
> (In reply to comment #107)
> > (From update of attachment 207777 [details] [details])
> > I haven't had time to review this yet but I have one question :) Would this patch fix some of the media/track tests? At least media/track/track-in-band.html ? It'd be great this is patch help unskip layout tests.
> 
> track-in-band.html expects the "kind" to be set to captions, but there's not currently any way to figure out the track kind in GStreamer (see: https://bugzilla.gnome.org/show_bug.cgi?id=698853). Also, it expects "forced" tracks, but I can't find them in the HTML5 spec.
> 

There is nothing in the spec about forced tracks yet, but this bug tracks adding something: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21667

[Brendan Long]

I have the following problems with the tests:

  * I can't use counting-captioned.mov, because GStreamer doesn't see the caption stream, and even if it did, it would get the kind wrong (since there's no way to tell the difference between captions and subtitles).

  * I can't use counting-subtitled.m4v, because Apple's tests expect that file to have "forced" subtitles, and there's no way to detect that in GStreamer either, even if we wanted to support it (which I think is outside the scope of this patch -- I really want to get the minimal version working before adding more).

I think I can fix this by creating a new counting-subtitled-no-forced.m4v file (is there any free software I can use to verify that my new file's subtitles aren't forced?). Is that the best way of handling this? It's kind of frustrating that none of the existing tests work because we need particular "kinds".

[Eric Carlson]

(In reply to comment #110)
> I have the following problems with the tests:
> 
>   * I can't use counting-captioned.mov, because GStreamer doesn't see the caption stream, and even if it did, it would get the kind wrong (since there's no way to tell the difference between captions and subtitles).
> 
>   * I can't use counting-subtitled.m4v, because Apple's tests expect that file to have "forced" subtitles, and there's no way to detect that in GStreamer either, even if we wanted to support it (which I think is outside the scope of this patch -- I really want to get the minimal version working before adding more).
> 
It sounds like GStreamer doesn't (yet) support CEA-608 tracks in .mov files. If it only supports subtitle tracks, can't you assume that all in-band tracks are kind=subtitle?

[Brendan Long]

(In reply to comment #111)
> It sounds like GStreamer doesn't (yet) support CEA-608 tracks in .mov files. If it only supports subtitle tracks, can't you assume that all in-band tracks are kind=subtitle?

Yes, that's what the patch does now (since it never gets a kind, it defaults to "subtitles"). The problem is that the counting-subtitled.m4v file also has "forced" subtitles, which we currently have no way of detecting, so both the -captioned.mov and -subtitled.m4v files cause problems with testing for GStreamer.

[Brendan Long]

Created attachment 209125 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/track/track-in-band-simple-cues-added-once.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 21 files


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

[Brendan Long]

This latest version includes a demonstration of what I meant by the difficulty with testing this. The new test is largely similar to track-in-band-cues-added-once.html, but without testing for "forced" subtitles, and ignoring the number of subtitles (because QtWebKit and Safari disagree).

Is this an acceptable way of handling the testing? I can make similar versions of the other tests (track-in-band.html, track-in-band-mode.html, track-in-band-style.html).

[Build Bot]

Comment on attachment 209125 [details]
Patch

Attachment 209125 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1523010

New failing tests:
media/track/track-in-band-simple-cues-added-once.html

[Build Bot]

Created attachment 209161 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209125 [details]
Patch

Attachment 209125 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1525050

New failing tests:
media/track/track-in-band-simple-cues-added-once.html

[Build Bot]

Created attachment 209183 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Eric Carlson]

(In reply to comment #115)
> This latest version includes a demonstration of what I meant by the difficulty with testing this. The new test is largely similar to track-in-band-cues-added-once.html, but without testing for "forced" subtitles, and ignoring the number of subtitles (because QtWebKit and Safari disagree).
> 
> Is this an acceptable way of handling the testing? I can make similar versions of the other tests (track-in-band.html, track-in-band-mode.html, track-in-band-style.html).

Would it work to have different results for different platforms?

[Brendan Long]

(In reply to comment #120)
> Would it work to have different results for different platforms?

That would work for track-in-band.html. For the other tests, it uses a .mov file, and GStreamer doesn't seem to detect the captions at the moment. Is there a way I could check in JavaScript which browser I'm testing and pick the video file based on that?

[Eric Carlson]

(In reply to comment #121)
> (In reply to comment #120)
> > Would it work to have different results for different platforms?
> 
> That would work for track-in-band.html. For the other tests, it uses a .mov file, and GStreamer doesn't seem to detect the captions at the moment. Is there a way I could check in JavaScript which browser I'm testing and pick the video file based on that?

MediaPlayer::engineDescription() would let you know. It isn't available to tests  now, but it should be easy enough to expose it via Internals object.

[Brendan Long]

Created attachment 209292 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/content/counting-subtitled-kate.ogv', u'LayoutTests/media/content/counting-subtitled-srt.mkv', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 32 files


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

[Brendan Long]

Ok, so the newest patch add two new files (a .mkv and a .ogv), then does tests on both of them. The idea is that individual ports can decide which in-band track formats they care about and want to test. I also made the tests each only test one thing (order, language attribute, kind attribute, mode attribute, style, whether cues are correct).

For some reason these tests always time out if I run them with run-webkit-tests, so I'm not sure if "style" works (because it requires "window.internals"). The mode tests are both failing at the moment (is it ok to check in failing tests?). track-in-band-kate-ogg-cues-added-once is also failing because that video seems to have issues if we seek (presumably unrelated to this patch -- I'll test in just a minute).

How should I arrange to make the mac builds skip these tests?

I'm going to look into turning VIDEO_TRACK on in QtWebKit, but runtime disabled, then use internals to turn it back on so this can be run consistently.

[Build Bot]

Comment on attachment 209292 [details]
Patch

Attachment 209292 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1514977

New failing tests:
media/track/in-band/track-in-band-kate-ogg-cues-added-once.html
media/track/in-band/track-in-band-srt-mkv-mode.html
media/track/in-band/track-in-band-kate-ogg-style.html
media/track/in-band/track-in-band-srt-mkv-track-order.html
media/track/in-band/track-in-band-kate-ogg-language.html
media/track/in-band/track-in-band-srt-mkv-cues-added-once.html
media/track/in-band/track-in-band-srt-mkv-language.html
media/track/in-band/track-in-band-kate-ogg-mode.html
media/track/in-band/track-in-band-kate-ogg-track-order.html
media/track/in-band/track-in-band-srt-mkv-kind.html
media/track/in-band/track-in-band-srt-mkv-style.html
media/track/in-band/track-in-band-kate-ogg-kind.html

[Build Bot]

Created attachment 209311 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Brendan Long]

Created attachment 209891 [details]
Add test expected files

This adds the -expected.txt files for the tests that pass. I also skipped the tests on mac. Unfortunately, the tests won't actually run on EWS for QtWebKit, since text tracks aren't enabled.

[Philippe Normand]

(In reply to comment #128)
> Created an attachment (id=209891) [details]
> Add test expected files
> 
> This adds the -expected.txt files for the tests that pass. I also skipped the tests on mac. Unfortunately, the tests won't actually run on EWS for QtWebKit, since text tracks aren't enabled.

Only the windows and mac EWS bots run the layout tests anyway.

[Brendan Long]

Created attachment 209897 [details]
Rebased previous patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/content/counting-subtitled-kate.ogv', u'LayoutTests/media/content/counting-subtitled-srt.mkv', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 40 files


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

[Eric Carlson]

Comment on attachment 209897 [details]
Rebased previous patch

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

I think this is close, but you definitely need to fix the layering violation. Please consider the other comments as well.

> Source/WebCore/ChangeLog:20
> +           media/track/in-band/track-in-band-kate-ogg-cues-added-once.html
> +           media/track/in-band/track-in-band-kate-ogg-kind.html
> +           media/track/in-band/track-in-band-kate-ogg-language.html
> +           media/track/in-band/track-in-band-kate-ogg-mode.html
> +           media/track/in-band/track-in-band-kate-ogg-style.html
> +           media/track/in-band/track-in-band-kate-ogg-track-order.html
> +           media/track/in-band/track-in-band-srt-mkv-cues-added-once.html
> +           media/track/in-band/track-in-band-srt-mkv-kind.html
> +           media/track/in-band/track-in-band-srt-mkv-language.html
> +           media/track/in-band/track-in-band-srt-mkv-mode.html
> +           media/track/in-band/track-in-band-srt-mkv-style.html
> +           media/track/in-band/track-in-band-srt-mkv-track-order.html

It *looks* like the kate and mkv feature tests are identical except for the media file. If so, please include the scripts from a .js file instead of duplicating them.

> Source/WebCore/html/track/InbandTextTrack.cpp:406
> +void InbandTextTrack::inbandTextTrackPrivateLabelChanged(InbandTextTrackPrivate* trackPrivate)

Nit: this is a method on InbandTextTrackPrivateClient so "inbandTextTrackPrivate" seems unnecessary in the method name.

Why require the track to call back into the media engine to get the label instead of passing it as a parameter?

> Source/WebCore/html/track/InbandTextTrack.cpp:413
> +void InbandTextTrack::inbandTextTrackPrivateLanguageChanged(InbandTextTrackPrivate* trackPrivate)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:171
> +        gboolean ret = gst_buffer_map(buffer, &info, GST_MAP_READ);
> +        ASSERT_UNUSED(ret, ret);

Is it worth have a "continue" if ret is non-zero in a release build.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:234
> +    m_cues.appendVector(cues);
> +    for (size_t i = 0; i < cues.size(); ++i)
> +        client()->addWebVTTCue(this, cues[i]);

Won't this re-add cues you have already added unless you clear m_cues?

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:84
> +    OwnPtr<WebVTTParser> m_webVTTParser;

The WebVTT parser is in html/track/ so this is a layering violation :-(. Luckily, I don't think it would be a big deal to pass the raw VTT bytes through the client interface and have the track do the parsing. It might make sense to subclass InbandTextTrack so every in-band track doesn't have get a VTT parser (I maybe should have done the same for "Generic" cues).

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:577
> +#if ENABLE(VIDEO_TRACK) && defined(GST_API_VERSION_1)
> +    /* Clear cues so we don't get duplicates */
> +    for (size_t i = 0; i < m_textTracks.size(); ++i)
> +        m_textTracks[i]->clearCues();
> +#endif

Do you really want to clear all in-band cues every time you seek? AVFoundation also delivers re-cues after a seek, but instead of flushing on a seek I just don't re-add a cue if it already exists, see InbandTextTrack::addGenericCue. This does mean that a track can't have two *identical* cues, but seems like an uncommon enough situation that it is worth the risk.

[Brendan Long]

(In reply to comment #132)
> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:84
> > +    OwnPtr<WebVTTParser> m_webVTTParser;
> 
> The WebVTT parser is in html/track/ so this is a layering violation :-(. Luckily, I don't think it would be a big deal to pass the raw VTT bytes through the client interface and have the track do the parsing. It might make sense to subclass InbandTextTrack so every in-band track doesn't have get a VTT parser (I maybe should have done the same for "Generic" cues).

Like this?

https://github.com/cablelabs/webkit/commit/43740486884c088b7c97ed077685560c14e423cf

Is adding a "cueFormat()" function the right way to handle this?

    RefPtr<InbandTextTrack> textTrack;
    switch (prpTrack->cueFormat()) {
    case InbandTextTrackPrivate::Generic:
        textTrack = InbandGenericTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, prpTrack);
        break;
    case InbandTextTrackPrivate::WebVTT:
        textTrack = InbandWebVTTTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, prpTrack);
        break;
    default:
        ASSERT_NOT_REACHED();
        // Make sure this gets GC'd
        RefPtr<InbandTextTrackPrivate> track = prpTrack;
        return;
    }

Also, I left InbandTextTrackPrivateClient mostly alone, but then added asserts for functions that shouldn't be called, like in InbandWebVTTTextTrack:

    virtual void addGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) OVERRIDE { ASSERT_NOT_REACHED(); }
    virtual void updateGenericCue(InbandTextTrackPrivate*, GenericCueData*) OVERRIDE { ASSERT_NOT_REACHED(); }
    virtual void removeGenericCue(InbandTextTrackPrivate*, GenericCueData*) OVERRIDE { ASSERT_NOT_REACHED(); }

Would it be better to have a InbandWebVTTTextTrackPrivateClient and InbandGenericTextTrackPrivateClient?

[Brendan Long]

Actually, is this necessary?

        // Make sure this gets GC'd
        RefPtr<InbandTextTrackPrivate> track = prpTrack;

I was under the impression that PassRefPtr didn't handle ref'ing, but the destructor makes it look like it does, so maybe I don't need to do this?

[Eric Carlson]

(In reply to comment #133)
> (In reply to comment #132)
> > > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h:84
> > > +    OwnPtr<WebVTTParser> m_webVTTParser;
> > 
> > The WebVTT parser is in html/track/ so this is a layering violation :-(. Luckily, I don't think it would be a big deal to pass the raw VTT bytes through the client interface and have the track do the parsing. It might make sense to subclass InbandTextTrack so every in-band track doesn't have get a VTT parser (I maybe should have done the same for "Generic" cues).
> 
> Like this?
> 
> https://github.com/cablelabs/webkit/commit/43740486884c088b7c97ed077685560c14e423cf
> 
> Is adding a "cueFormat()" function the right way to handle this?
> 
>     RefPtr<InbandTextTrack> textTrack;
>     switch (prpTrack->cueFormat()) {
>     case InbandTextTrackPrivate::Generic:
>         textTrack = InbandGenericTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, prpTrack);
>         break;
>     case InbandTextTrackPrivate::WebVTT:
>         textTrack = InbandWebVTTTextTrack::create(ActiveDOMObject::scriptExecutionContext(), this, prpTrack);
>         break;
>     default:
>         ASSERT_NOT_REACHED();
>         // Make sure this gets GC'd
>         RefPtr<InbandTextTrackPrivate> track = prpTrack;
>         return;
>     }
> 
  Yes, although I think this should be in InbandTextTrack::create instead so HTMLMediaElement doesn't have to know about the different types of in-band tracks.

> Also, I left InbandTextTrackPrivateClient mostly alone, but then added asserts for functions that shouldn't be called, like in InbandWebVTTTextTrack:
> 
>     virtual void addGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) OVERRIDE { ASSERT_NOT_REACHED(); }
>     virtual void updateGenericCue(InbandTextTrackPrivate*, GenericCueData*) OVERRIDE { ASSERT_NOT_REACHED(); }
>     virtual void removeGenericCue(InbandTextTrackPrivate*, GenericCueData*) OVERRIDE { ASSERT_NOT_REACHED(); }
> 
> Would it be better to have a InbandWebVTTTextTrackPrivateClient and InbandGenericTextTrackPrivateClient?

  This seems fine.

[Eric Carlson]

(In reply to comment #134)
> Actually, is this necessary?
> 
>         // Make sure this gets GC'd
>         RefPtr<InbandTextTrackPrivate> track = prpTrack;
> 
> I was under the impression that PassRefPtr didn't handle ref'ing, but the destructor makes it look like it does, so maybe I don't need to do this?

  It is not necessary.

[Brendan Long]

Would it be bad if I made InbandTextTrackPrivate::cueFormat() a pure virtual function and removed InbandTextTrackPrivate::create()?

Every subclass should pick a CueFormat, and it would be convenient if we could be assured of that at compile-time, but nothing else in that class is pure-virtual, and the existence of the create() function confuses me.

A grep shows that it's not used though:

$ grep -r InbandTextTrackPrivate::create Source
# nothing

Another option would be to make InbandTextTrackPrivate::cueFormat() default to Generic.

[Brendan Long]

(In reply to comment #137)
> Would it be bad if I made InbandTextTrackPrivate::cueFormat() a pure virtual function and removed InbandTextTrackPrivate::create()?

Actually, I found a better way. I made it work like TrackBase, where you pass the type to the constructor. I still removed InbandTextTrackPrivate::create() since it's unused and not very useful.

[Brendan Long]

Created attachment 210019 [details]
Split generic and WebVTT tracks, move WebVTTParser to InbandWebVTTTextTrack, move tests into .js files, add more expected.txt files, skip duplicate cues instead of deleting when we seek, and simplify language and label callbacks.

Here another patch, which I think incorporates all of the changes Eric suggested. I also figured out why the tests were failing for me (I needed to add --wrapper=gst-git), so now all of the tests I added usually pass for QtWebKit (although some of them are pretty flakey).

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/content/counting-subtitled-kate.ogv', u'LayoutTests/media/content/counting-subtitled-srt.mkv', u'LayoutTests/media/track/in-band/test-attribute.js', u'LayoutTests/media/track/in-band/test-cues-added-once.js', u'LayoutTests/media/track/in-band/test-mode.js', u'LayoutTests/media/track/in-band/test-style.js', u'LayoutTests/media/track/in-band/test-track-order.js', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.h', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/html/track/InbandWebVTTTextTrack.cpp', u'Source/WebCore/html/track/InbandWebVTTTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivate.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 58 files


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

[Build Bot]

Comment on attachment 210019 [details]
Split generic and WebVTT tracks, move WebVTTParser to InbandWebVTTTextTrack, move tests into .js files, add more expected.txt files, skip duplicate cues instead of deleting when we seek, and simplify language and label callbacks.

Attachment 210019 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1605697

[Build Bot]

Comment on attachment 210019 [details]
Split generic and WebVTT tracks, move WebVTTParser to InbandWebVTTTextTrack, move tests into .js files, add more expected.txt files, skip duplicate cues instead of deleting when we seek, and simplify language and label callbacks.

Attachment 210019 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1643071

[Brendan Long]

Created attachment 210023 [details]
Fix Mac build.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/content/counting-subtitled-kate.ogv', u'LayoutTests/media/content/counting-subtitled-srt.mkv', u'LayoutTests/media/track/in-band/test-attribute.js', u'LayoutTests/media/track/in-band/test-cues-added-once.js', u'LayoutTests/media/track/in-band/test-mode.js', u'LayoutTests/media/track/in-band/test-style.js', u'LayoutTests/media/track/in-band/test-track-order.js', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.h', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/html/track/InbandWebVTTTextTrack.cpp', u'Source/WebCore/html/track/InbandWebVTTTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivate.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 59 files


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

[Brendan Long]

Created attachment 210033 [details]
Also add Windows build

Not sure if this is necessary, but InbandTextTrack.{cpp,h} is there, so I figure the generic and WebVTT versions should be too.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/content/counting-subtitled-kate.ogv', u'LayoutTests/media/content/counting-subtitled-srt.mkv', u'LayoutTests/media/track/in-band/test-attribute.js', u'LayoutTests/media/track/in-band/test-cues-added-once.js', u'LayoutTests/media/track/in-band/test-mode.js', u'LayoutTests/media/track/in-band/test-style.js', u'LayoutTests/media/track/in-band/test-track-order.js', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.h', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/html/track/InbandWebVTTTextTrack.cpp', u'Source/WebCore/html/track/InbandWebVTTTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivate.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 61 files


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

[Jer Noble]

Comment on attachment 210033 [details]
Also add Windows build

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

> Source/WebCore/html/track/InbandGenericTextTrack.cpp:156
> +        LOG(Media, "InbandTextTrack::addGenericCue ignoring already added cue: start=%.2f, end=%.2f, content=\"%s\"\n", cueData->startTime(), cueData->endTime(), cueData->content().utf8().data());

This should probably be fixed to log the new class name.

> Source/WebCore/html/track/InbandGenericTextTrack.cpp:182
> +        LOG(Media, "InbandTextTrack::removeGenericCue removing cue: start=%.2f, end=%.2f, content=\"%s\"\n", cueData->startTime(), cueData->endTime(), cueData->content().utf8().data());

Ditto.

> Source/WebCore/html/track/InbandWebVTTTextTrack.h:56
> +class WebVTTTextTrackCueMap {
> +public:
> +    WebVTTTextTrackCueMap();
> +    virtual ~WebVTTTextTrackCueMap();
> +
> +    void add(WebVTTCueData*, TextTrackCue*);
> +
> +    void remove(TextTrackCue*);
> +    void remove(WebVTTCueData*);
> +
> +    PassRefPtr<WebVTTCueData> find(TextTrackCue*);
> +    PassRefPtr<TextTrackCue> find(WebVTTCueData*);
> +
> +private:
> +    typedef HashMap<RefPtr<TextTrackCue>, RefPtr<WebVTTCueData> > CueToDataMap;
> +    typedef HashMap<RefPtr<WebVTTCueData>, RefPtr<TextTrackCue> > CueDataToCueMap;
> +
> +    CueToDataMap m_cueToDataMap;
> +    CueDataToCueMap m_dataToCueMap;
> +};

It might be nice to templatize this class, since it's essentially duplicated in both this and InbandGenericTextTrack.

[Brendan Long]

(In reply to comment #147)
> It might be nice to templatize this class, since it's essentially duplicated in both this and InbandGenericTextTrack.

Should I move the implementation into InbandTextTrack.h like a normal template, or put it in InbandTextTrack.cpp and instantiate the templates there?

[Eric Carlson]

Comment on attachment 210033 [details]
Also add Windows build

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

The only real problem I see is the unnecessary map in InbandWebVTTTextTrack, but I think that makes it worth going one more round.

> Source/WebCore/ChangeLog:73
> +        (WTF::adoptGRef):
> +        (WTF::GstSample):
> +        (WTF::GstEvent):

Nit: entries that don't need a comment should be removed.

> Source/WebCore/ChangeLog:77
> +        (webkitGstCheckVersion):

Ditto.

> Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:96
> +    , m_webVTTParser(WebVTTParser::create(this, context))

The track might not ever get any cue data so why don't you allocate the parser the first time parseWebVTTCueData is called?

> Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:105
> +void InbandWebVTTTextTrack::parseWebVTTCueData(InbandTextTrackPrivate* trackPrivate,
> +    const char* data, unsigned length)

Nit: the line wrap it unnecessary.

> Source/WebCore/html/track/InbandWebVTTTextTrack.cpp:135
> +        m_cueMap.add(cueData.get(), cue.get());

I don't think you need a cue map for this class. With "generic" cues, we don't know a cue's duration until the next cue (or empty cue) is delivered. We don't want to delay adding them to the cue tree, or we might not display them on time (or at all), so they are initially delivered with an infinite duration and then updateGenericCue() is called when we know the duration. This means we need to have a map to find the TextTrackCue that is already in the tree from the GenericCueData, and thus the map. A cue is removed from the map ocne it gets a duration.

In this case you have the entire WebVTT cue so will always have the duration and the map should be unnecessary.

>> Source/WebCore/html/track/InbandWebVTTTextTrack.h:56
>> +};
> 
> It might be nice to templatize this class, since it's essentially duplicated in both this and InbandGenericTextTrack.

Or remove it completely ;-)

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:145
> +    virtual void labelChanged(InbandTextTrackPrivate*, const String& label) = 0;
> +    virtual void languageChanged(InbandTextTrackPrivate*, const String& language) = 0;

Nit: these variable names aren't really necessary.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:110
> +    if (m_tagTimerHandler)
> +        g_source_remove(m_tagTimerHandler);

Should you zero m_tagTimerHandler as well?

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:201
> +        m_label = emptyAtom;
> +        m_language = emptyAtom;

If the track previously had a label/language, InbandTextTrack will now be out of sync.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:212
> +        m_label = emptyAtom;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:220
> +        m_language = emptyAtom;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:645
> +    m_textTimerHandler = 0;

Do you not need to call g_source_remove() here as well?

> LayoutTests/ChangeLog:11
> +        (.canplaythrough):
> +        (testAttribute):

These empty entries can be removed.

> LayoutTests/ChangeLog:22
> +        * media/track/in-band/test-cues-added-once.js: Added.
> +        (testCuesAddedOnce):
> +        * media/track/in-band/test-mode.js: Added.
> +        (testMode.seeked):
> +        (testMode.canplaythrough):
> +        (testMode):
> +        * media/track/in-band/test-style.js: Added.
> +        (testStyle.seeked):
> +        (testStyle.canplaythrough):
> +        (testStyle):
> +        * media/track/in-band/test-track-order.js: Added.

Each of these .js files only has a single function so they could be combined into a single "in-band-cues.js" (or whatever) file. It should probably go into media/ along with the other .js files used for media tests.

[Brendan Long]

I made most of the changes suggested, except:

(In reply to comment #149)
> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:110
> > +    if (m_tagTimerHandler)
> > +        g_source_remove(m_tagTimerHandler);
> 
> Should you zero m_tagTimerHandler as well?

It doesn't really matter because that function starts with:

    if (m_isDisconnected)
        return;

    m_isDisconnected = true;

I think we don't actually need m_isDisconnected anymore though (I think it was originally useful for something else), so I changed it to this:

    if (!m_pad)
        return;

Which has the same effect.

> > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:201
> > +        m_label = emptyAtom;
> > +        m_language = emptyAtom;
> 
> If the track previously had a label/language, InbandTextTrack will now be out of sync.

I fixed this so it sends events when the label and language change, instead of every time we see non-empty tags:

    String label;
    String language;
    if (tags) {
        gchar* tagValue;
        if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &tagValue)) {
            INFO_MEDIA_MESSAGE("Track %d got title %s.", m_index, tagValue);
            label = tagValue;
            g_free(tagValue);
        }

        if (gst_tag_list_get_string(tags, GST_TAG_LANGUAGE_CODE, &tagValue)) {
            INFO_MEDIA_MESSAGE("Track %d got language %s.", m_index, tagValue);
            language = tagValue;
            g_free(tagValue);
        }
    }

    if (m_label != label) {
        m_label = label;
        client()->labelChanged(this, m_label);
    }

    if (m_language != language) {
        m_language = language;
        client()->languageChanged(this, m_language);
    }

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:645
> > +    m_textTimerHandler = 0;
> 
> Do you not need to call g_source_remove() here as well?

No, `mediaPlayerPrivateTextChangeTimeoutCallback` returns FALSE. From the documentation for g_timeout_add:

> The function is called repeatedly until it returns FALSE, at which point the timeout is automatically destroyed and the function will not be called again. 

For these functions, I followed what was already being done in MediaPlayerPrivateGStreamer, but it's possible that this:

    void MediaPlayerPrivateGStreamer::textChanged()
    {
        if (m_textTimerHandler)
            g_source_remove(m_textTimerHandler);
        m_textTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateTextChangeTimeoutCallback), this);
    }

Could be this:

    void MediaPlayerPrivateGStreamer::textChanged()
    {
        if (!m_textTimerHandler)
            m_textTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateTextChangeTimeoutCallback), this);
    }

I *think* rescheduling the event is useful though, since we tend to get all of the tracks at once, and pushing the event back every time we see one lets us consolidate all of those into one event.

[Eric Carlson]

(In reply to comment #150)
> I made most of the changes suggested, except:
> 
> (In reply to comment #149)
> > > Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp:201
> > > +        m_label = emptyAtom;
> > > +        m_language = emptyAtom;
> > 
> > If the track previously had a label/language, InbandTextTrack will now be out of sync.
> 
> I fixed this so it sends events when the label and language change, instead of every time we see non-empty tags:
> 
>     String label;
>     String language;
>     if (tags) {
>         gchar* tagValue;
>         if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &tagValue)) {
>             INFO_MEDIA_MESSAGE("Track %d got title %s.", m_index, tagValue);
>             label = tagValue;
>             g_free(tagValue);
>         }
> 
>         if (gst_tag_list_get_string(tags, GST_TAG_LANGUAGE_CODE, &tagValue)) {
>             INFO_MEDIA_MESSAGE("Track %d got language %s.", m_index, tagValue);
>             language = tagValue;
>             g_free(tagValue);
>         }
>     }
> 
>     if (m_label != label) {
>         m_label = label;
>         client()->labelChanged(this, m_label);
>     }
> 
>     if (m_language != language) {
>         m_language = language;
>         client()->languageChanged(this, m_language);
>     }
> 
Much cleaner, thanks.

> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:645
> > > +    m_textTimerHandler = 0;
> > 
> > Do you not need to call g_source_remove() here as well?
> 
> No, `mediaPlayerPrivateTextChangeTimeoutCallback` returns FALSE. From the documentation for g_timeout_add:
> 
> > The function is called repeatedly until it returns FALSE, at which point the timeout is automatically destroyed and the function will not be called again. 
> 
> For these functions, I followed what was already being done in MediaPlayerPrivateGStreamer, but it's possible that this:
> 
>     void MediaPlayerPrivateGStreamer::textChanged()
>     {
>         if (m_textTimerHandler)
>             g_source_remove(m_textTimerHandler);
>         m_textTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateTextChangeTimeoutCallback), this);
>     }
> 
> Could be this:
> 
>     void MediaPlayerPrivateGStreamer::textChanged()
>     {
>         if (!m_textTimerHandler)
>             m_textTimerHandler = g_timeout_add(0, reinterpret_cast<GSourceFunc>(mediaPlayerPrivateTextChangeTimeoutCallback), this);
>     }
> 
> I *think* rescheduling the event is useful though, since we tend to get all of the tracks at once, and pushing the event back every time we see one lets us consolidate all of those into one event.

I know essentially nothing about the GTK API, so it was just an idle question. Thanks for the explanation!

[Brendan Long]

I'm getting this when I try to upload the patch:

Adding patch "Fix comments and changelogs, remove WebVTTTextTrackCueMap, fix label and language events, instantiate WebVTTParser on-demand, remove label and language variable names from InbandTextTrackPrivateClient, remove m_isDisconnected and combine cue tests into in-band-cues.js" to https://bugs.webkit.org/show_bug.cgi?id=103771
Traceback (most recent call last):
  File "Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 54, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/tool/steps/postdiff.py", line 50, in run
    self._tool.bugs.add_patch_to_bug(bug_id, diff, description, comment_text=comment_text, mark_for_review=self._options.review, mark_for_commit_queue=self._options.request_commit)
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 628, in add_patch_to_bug
    self.browser.submit()
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 541, in submit
    return self.open(self.click(*args, **kwds))
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/home/blong/workspace/webkit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

I'll see if I can upload it manually.

[Brendan Long]

Created attachment 210148 [details]
Fix comments and changelogs, remove WebVTTTextTrackCueMap, fix label and language events, instantiate WebVTTParser on-demand, remove label and language variable names from InbandTextTrackPrivateClient

[Brendan Long]

Created attachment 210151 [details]
Previous patch, with --binary

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/content/counting-subtitled-kate.ogv', u'LayoutTests/media/content/counting-subtitled-srt.mkv', u'LayoutTests/media/in-band-cues.js', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-kind.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-language.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-mode.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-style.html', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-kate-ogg-track-order.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-cues-added-once.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-kind.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-language.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-mode.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-style.html', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order-expected.txt', u'LayoutTests/media/track/in-band/track-in-band-srt-mkv-track-order.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.cpp', u'Source/WebCore/html/track/InbandGenericTextTrack.h', u'Source/WebCore/html/track/InbandTextTrack.cpp', u'Source/WebCore/html/track/InbandTextTrack.h', u'Source/WebCore/html/track/InbandWebVTTTextTrack.cpp', u'Source/WebCore/html/track/InbandWebVTTTextTrack.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivate.h', u'Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h', u'Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  webkit_text_combiner_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:187:  webkit_text_combiner_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:45:  webkit_text_sink_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextSinkGStreamer.cpp:79:  webkit_text_sink_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 57 files


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

[Eric Carlson]

Comment on attachment 210151 [details]
Previous patch, with --binary

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

> LayoutTests/media/in-band-cues.js:144
> +function testTrackOrder(uri) {

Nit: this brace should be on a new line. Lets fix this in a future check in.

[WebKit Commit Bot]

Comment on attachment 210151 [details]
Previous patch, with --binary

Clearing flags on attachment: 210151

Committed r154908: <http://trac.webkit.org/changeset/154908>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brendan Long]

I suspect anyone who was following this bug may be interested in these two also.

This first one has a patch that needs review:

[GStreamer] Support "chapter" text tracks:
https://bugs.webkit.org/show_bug.cgi?id=122000

This one will be largely similar to the above patch:

[GStreamer] Expose MPEG-TS metadata:
https://bugs.webkit.org/show_bug.cgi?id=122001