Bug 103771

Summary: [GStreamer] support in-band text tracks
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: b.long, buildbot, commit-queue, david.corvoysier, eflews.bot, esprehn+autocc, glenn, gtk-ews, gustavo, gyuyoung.kim, gyuyoung.kim, jer.noble, jturcotte, kbalazs, laszlo.gombos, menard, mrobinson, philn, pnormand, rakuco, rego+ews, rniwa, sjadaud, slomo, stephanepublic, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
uridecodebin+playsink
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
EFL support
none
Fix assert misuse and unused variable warnings
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Fix build warnings
none
Fix textTrackIndex() function
none
Put InbandTextTrackPrivateGStreamer in webcore_platform_sources
none
Pipeline while playing
none
Output from adding a probe after the webvttenc and before the funnel.
none
Unref pad after usage, use static casts, change license on new files to BSD
none
Extend appsink to prevent playsink from setting sync=true.
none
Fix properties to start at 0
none
Fix some more leaks with adoptGRef()
none
Patch
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Add test expected files
none
Rebased previous patch
none
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.
none
Fix Mac build.
none
Also add Windows build
none
Fix comments and changelogs, remove WebVTTTextTrackCueMap, fix label and language events, instantiate WebVTTParser on-demand, remove label and language variable names from InbandTextTrackPrivateClient
none
Previous patch, with --binary none

Description Eric Carlson 2012-11-30 13:28:45 PST
In-band caption track support requires changes in the media engine.
Comment 1 Philippe Normand 2013-02-10 06:24:46 PST
*** Bug 109378 has been marked as a duplicate of this bug. ***
Comment 2 Brendan Long 2013-04-04 14:11:38 PDT
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?
Comment 3 Brendan Long 2013-04-04 14:17:27 PDT
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?
Comment 4 Eric Carlson 2013-04-04 14:18:43 PDT
(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.
Comment 5 Philippe Normand 2013-04-04 14:20:36 PDT
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 :(
Comment 6 Brendan Long 2013-04-04 14:41:17 PDT
(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.
Comment 7 Sebastian Dröge (slomo) 2013-04-05 04:42:48 PDT
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.
Comment 8 Philippe Normand 2013-04-05 06:32:54 PDT
Brendan, for audio/video tracks support please refer to bug 113514. Bug 103771 is about text tracks
Comment 9 Brendan Long 2013-04-05 07:40:19 PDT
(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.
Comment 10 Sebastian Dröge (slomo) 2013-04-05 09:22:55 PDT
(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)
Comment 11 Brendan Long 2013-04-05 10:23:34 PDT
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.
Comment 12 Brendan Long 2013-04-10 16:06:01 PDT
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
Comment 13 Brendan Long 2013-04-24 15:37:20 PDT
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.
Comment 14 Martin Robinson 2013-04-24 15:40:36 PDT
(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.
Comment 15 Brendan Long 2013-04-24 15:41:51 PDT
(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?
Comment 16 Martin Robinson 2013-04-24 15:44:46 PDT
(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. :)
Comment 17 Brendan Long 2013-04-24 15:48:17 PDT
Created attachment 199529 [details]
uridecodebin+playsink
Comment 18 Brendan Long 2013-04-24 15:50:32 PDT
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.
Comment 19 Build Bot 2013-04-24 16:21:46 PDT
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
Comment 20 Build Bot 2013-04-24 16:21:48 PDT
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
Comment 21 EFL EWS Bot 2013-04-24 16:39:53 PDT
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
Comment 22 EFL EWS Bot 2013-04-24 16:48:17 PDT
Comment on attachment 199529 [details]
uridecodebin+playsink

Attachment 199529 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/114482
Comment 23 Sebastian Dröge (slomo) 2013-04-25 00:27:23 PDT
(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.
Comment 24 Sebastian Dröge (slomo) 2013-04-25 00:29:34 PDT
(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
Comment 25 Philippe Normand 2013-04-25 00:45:25 PDT
(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.
Comment 26 Sebastian Dröge (slomo) 2013-04-25 01:11:43 PDT
(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, ...)
Comment 27 Brendan Long 2013-04-25 08:15:20 PDT
(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.
Comment 28 Brendan Long 2013-04-25 08:18:24 PDT
One more question: Is there a way to figure out the track "kind" in GStreamer? Should there be a GST_TAG_KIND?
Comment 29 Sebastian Dröge (slomo) 2013-04-25 08:38:02 PDT
(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.
Comment 30 Brendan Long 2013-04-25 08:41:46 PDT
(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
Comment 31 Sebastian Dröge (slomo) 2013-04-25 08:45:13 PDT
(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
Comment 32 Brendan Long 2013-04-25 08:47:30 PDT
I created this bug against GStreamer: https://bugzilla.gnome.org/show_bug.cgi?id=698851
Comment 33 Brendan Long 2013-04-25 08:59:24 PDT
And this for the "kind" attribute: https://bugzilla.gnome.org/show_bug.cgi?id=698853
Comment 34 Brendan Long 2013-04-25 09:03:41 PDT
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.
Comment 35 Brendan Long 2013-04-25 09:19:56 PDT
Created attachment 199670 [details]
EFL support
Comment 36 Brendan Long 2013-04-25 09:20:50 PDT
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.
Comment 37 EFL EWS Bot 2013-04-25 09:26:43 PDT
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
Comment 38 EFL EWS Bot 2013-04-25 09:28:53 PDT
Comment on attachment 199670 [details]
EFL support

Attachment 199670 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/171307
Comment 39 Sebastian Dröge (slomo) 2013-04-25 09:32:54 PDT
(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
Comment 40 Brendan Long 2013-04-25 09:46:11 PDT
Created attachment 199675 [details]
Fix assert misuse and unused variable warnings
Comment 41 EFL EWS Bot 2013-04-25 10:23:11 PDT
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
Comment 42 EFL EWS Bot 2013-04-25 10:23:40 PDT
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
Comment 43 Build Bot 2013-04-25 11:12:50 PDT
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
Comment 44 Build Bot 2013-04-25 11:12:58 PDT
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
Comment 45 Brendan Long 2013-05-02 11:22:50 PDT
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.
Comment 46 Philippe Normand 2013-05-03 08:07:14 PDT
(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 :)
Comment 47 Brendan Long 2013-05-03 08:22:43 PDT
(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.
Comment 48 Philippe Normand 2013-05-30 01:03:37 PDT
Hi Brendan,

So now that you extended playbin I suppose this patch can be revamped? :)
Comment 49 Brendan Long 2013-05-30 08:08:09 PDT
(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.
Comment 50 Brendan Long 2013-05-30 12:28:32 PDT
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.
Comment 51 Brendan Long 2013-06-28 08:52:59 PDT
I'm working on an element to combine the text streams: https://bugzilla.gnome.org/show_bug.cgi?id=703267
Comment 52 Brendan Long 2013-07-15 14:51:29 PDT
Created attachment 206687 [details]
Patch
Comment 53 WebKit Commit Bot 2013-07-15 14:54:23 PDT
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.
Comment 54 EFL EWS Bot 2013-07-15 14:55:38 PDT
Comment on attachment 206687 [details]
Patch

Attachment 206687 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1071578
Comment 55 EFL EWS Bot 2013-07-15 14:56:58 PDT
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
Comment 56 kov's GTK+ EWS bot 2013-07-15 15:02:37 PDT
Comment on attachment 206687 [details]
Patch

Attachment 206687 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1069622
Comment 57 Brendan Long 2013-07-15 15:03:36 PDT
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?).
Comment 58 Philippe Normand 2013-07-16 00:13:16 PDT
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.
Comment 59 Brendan Long 2013-07-16 08:29:45 PDT
(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.
Comment 60 Brendan Long 2013-07-16 15:07:45 PDT
Created attachment 206816 [details]
Patch
Comment 61 Brendan Long 2013-07-16 15:08:02 PDT
Now it should build..
Comment 62 WebKit Commit Bot 2013-07-16 15:09:58 PDT
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.
Comment 63 EFL EWS Bot 2013-07-16 15:13:47 PDT
Comment on attachment 206816 [details]
Patch

Attachment 206816 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1114064
Comment 64 EFL EWS Bot 2013-07-16 15:18:10 PDT
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
Comment 65 kov's GTK+ EWS bot 2013-07-16 15:20:37 PDT
Comment on attachment 206816 [details]
Patch

Attachment 206816 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1091303
Comment 66 Brendan Long 2013-07-16 15:26:25 PDT
Created attachment 206818 [details]
Fix build warnings
Comment 67 Brendan Long 2013-07-16 15:27:11 PDT
Is there a way to make QtWebKit build with the same warnings / errors as WebKitGTK and EWebKit?
Comment 68 WebKit Commit Bot 2013-07-16 15:27:25 PDT
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.
Comment 69 kov's GTK+ EWS bot 2013-07-16 15:34:45 PDT
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
Comment 70 Brendan Long 2013-07-16 15:44:58 PDT
Created attachment 206820 [details]
Fix textTrackIndex() function
Comment 71 WebKit Commit Bot 2013-07-16 15:46:58 PDT
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.
Comment 72 kov's GTK+ EWS bot 2013-07-16 15:58:25 PDT
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
Comment 73 Brendan Long 2013-07-16 16:07:07 PDT
> 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?
Comment 74 Philippe Normand 2013-07-17 06:41:36 PDT
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.
Comment 75 Brendan Long 2013-07-17 10:44:57 PDT
Created attachment 206900 [details]
Put InbandTextTrackPrivateGStreamer in webcore_platform_sources
Comment 76 WebKit Commit Bot 2013-07-17 10:47:21 PDT
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.
Comment 77 Brendan Long 2013-07-17 11:00:25 PDT
(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.
Comment 78 Brendan Long 2013-07-17 13:24:18 PDT
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".
Comment 79 Brendan Long 2013-07-17 13:43:43 PDT
For the qtdemux thing, it looks like it was completely ignoring disabled streams, and like usual.. someone else just fixed it.
Comment 80 Brendan Long 2013-07-17 13:58:44 PDT
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.
Comment 81 Brendan Long 2013-07-17 16:35:34 PDT
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
Comment 82 Philippe Normand 2013-07-17 23:41:04 PDT
(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
Comment 83 Philippe Normand 2013-07-17 23:57:42 PDT
(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
Comment 84 Brendan Long 2013-07-22 12:52:13 PDT
(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.
Comment 85 Brendan Long 2013-07-22 14:47:42 PDT
(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..
Comment 86 Sebastian Dröge (slomo) 2013-07-23 00:17:51 PDT
(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.
Comment 87 Brendan Long 2013-07-23 09:58:36 PDT
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.
Comment 88 Sebastian Dröge (slomo) 2013-07-23 23:50:45 PDT
(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.
Comment 89 Brendan Long 2013-07-24 08:06:40 PDT
(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.
Comment 90 Sebastian Dröge (slomo) 2013-07-24 09:00:13 PDT
(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? :)
Comment 91 Brendan Long 2013-07-24 09:01:44 PDT
(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.
Comment 92 Sebastian Dröge (slomo) 2013-07-24 09:11:34 PDT
(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 :)
Comment 93 Sebastian Dröge (slomo) 2013-07-24 09:12:27 PDT
(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.
Comment 94 Brendan Long 2013-07-24 16:21:40 PDT
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.
Comment 95 Sebastian Dröge (slomo) 2013-07-24 23:35:14 PDT
(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
Comment 96 Brendan Long 2013-07-25 07:47:10 PDT
(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);
Comment 97 Brendan Long 2013-07-25 11:26:54 PDT
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..
Comment 98 WebKit Commit Bot 2013-07-25 11:28:40 PDT
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.
Comment 99 Sebastian Dröge (slomo) 2013-07-26 00:44:57 PDT
(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? :)
Comment 100 Brendan Long 2013-07-26 11:07:51 PDT
(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.
Comment 101 Brendan Long 2013-07-30 12:18:55 PDT
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.
Comment 102 WebKit Commit Bot 2013-07-30 12:21:45 PDT
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.
Comment 103 Brendan Long 2013-07-30 13:27:58 PDT
Created attachment 207761 [details]
Fix properties to start at 0
Comment 104 WebKit Commit Bot 2013-07-30 13:31:15 PDT
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.
Comment 105 Brendan Long 2013-07-30 15:46:13 PDT
Created attachment 207777 [details]
Fix some more leaks with adoptGRef()
Comment 106 WebKit Commit Bot 2013-07-30 15:49:41 PDT
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.
Comment 107 Philippe Normand 2013-08-08 08:12:08 PDT
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.
Comment 108 Brendan Long 2013-08-08 08:45:02 PDT
(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..
Comment 109 Eric Carlson 2013-08-08 08:54:38 PDT
(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
Comment 110 Brendan Long 2013-08-08 10:53:09 PDT
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".
Comment 111 Eric Carlson 2013-08-08 11:12:03 PDT
(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?
Comment 112 Brendan Long 2013-08-08 11:18:10 PDT
(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.
Comment 113 Brendan Long 2013-08-19 14:44:29 PDT
Created attachment 209125 [details]
Patch
Comment 114 WebKit Commit Bot 2013-08-19 14:46:29 PDT
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.
Comment 115 Brendan Long 2013-08-19 14:47:36 PDT
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).
Comment 116 Build Bot 2013-08-19 23:02:02 PDT
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
Comment 117 Build Bot 2013-08-19 23:02:10 PDT
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
Comment 118 Build Bot 2013-08-20 05:29:29 PDT
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
Comment 119 Build Bot 2013-08-20 05:29:38 PDT
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
Comment 120 Eric Carlson 2013-08-20 08:11:55 PDT
(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?
Comment 121 Brendan Long 2013-08-20 08:19:02 PDT
(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?
Comment 122 Eric Carlson 2013-08-20 09:06:00 PDT
(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.
Comment 123 Brendan Long 2013-08-21 12:16:03 PDT
Created attachment 209292 [details]
Patch
Comment 124 WebKit Commit Bot 2013-08-21 12:19:10 PDT
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.
Comment 125 Brendan Long 2013-08-21 12:21:15 PDT
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.
Comment 126 Build Bot 2013-08-21 16:14:26 PDT
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
Comment 127 Build Bot 2013-08-21 16:14:36 PDT
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
Comment 128 Brendan Long 2013-08-28 09:07:46 PDT
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.
Comment 129 Philippe Normand 2013-08-28 09:10:34 PDT
(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.
Comment 130 Brendan Long 2013-08-28 09:36:40 PDT
Created attachment 209897 [details]
Rebased previous patch
Comment 131 WebKit Commit Bot 2013-08-28 09:38:40 PDT
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.
Comment 132 Eric Carlson 2013-08-28 10:54:57 PDT
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.
Comment 133 Brendan Long 2013-08-28 16:50:17 PDT
(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?
Comment 134 Brendan Long 2013-08-28 16:53:21 PDT
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?
Comment 135 Eric Carlson 2013-08-29 07:50:23 PDT
(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.
Comment 136 Eric Carlson 2013-08-29 07:51:57 PDT
(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.
Comment 137 Brendan Long 2013-08-29 09:18:28 PDT
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.
Comment 138 Brendan Long 2013-08-29 09:35:43 PDT
(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.
Comment 139 Brendan Long 2013-08-29 13:01:41 PDT
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).
Comment 140 WebKit Commit Bot 2013-08-29 13:05:23 PDT
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.
Comment 141 Build Bot 2013-08-29 13:09:40 PDT
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
Comment 142 Build Bot 2013-08-29 13:26:51 PDT
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
Comment 143 Brendan Long 2013-08-29 13:34:02 PDT
Created attachment 210023 [details]
Fix Mac build.
Comment 144 WebKit Commit Bot 2013-08-29 13:39:07 PDT
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.
Comment 145 Brendan Long 2013-08-29 14:39:22 PDT
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.
Comment 146 WebKit Commit Bot 2013-08-29 14:43:33 PDT
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.
Comment 147 Jer Noble 2013-08-30 09:41:27 PDT
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.
Comment 148 Brendan Long 2013-08-30 10:12:17 PDT
(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?
Comment 149 Eric Carlson 2013-08-30 10:15:27 PDT
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.
Comment 150 Brendan Long 2013-08-30 10:59:43 PDT
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.
Comment 151 Eric Carlson 2013-08-30 11:05:42 PDT
(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!
Comment 152 Brendan Long 2013-08-30 11:40:36 PDT
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.
Comment 153 Brendan Long 2013-08-30 12:32:57 PDT
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
Comment 154 Brendan Long 2013-08-30 12:37:01 PDT
Created attachment 210151 [details]
Previous patch, with --binary
Comment 155 WebKit Commit Bot 2013-08-30 12:41:28 PDT
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.
Comment 156 Eric Carlson 2013-08-30 12:50:55 PDT
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.
Comment 157 WebKit Commit Bot 2013-08-30 13:16:30 PDT
Comment on attachment 210151 [details]
Previous patch, with --binary

Clearing flags on attachment: 210151

Committed r154908: <http://trac.webkit.org/changeset/154908>
Comment 158 WebKit Commit Bot 2013-08-30 13:16:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 159 Brendan Long 2013-09-30 10:03:33 PDT
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