Bug 117039 - [GStreamer] Support audio and video tracks
Summary: [GStreamer] Support audio and video tracks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brendan Long
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-30 11:58 PDT by Brendan Long
Modified: 2013-11-01 09:34 PDT (History)
21 users (show)

See Also:


Attachments
Patch (41.00 KB, patch)
2013-05-30 12:03 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Remove unnecessary old code and add #if ENABLE(VIDEO_TRACKS) when removing audio and video tracks (40.38 KB, patch)
2013-05-30 12:18 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
ReFix comparison of signed and unsigned and disconnected() function (40.36 KB, patch)
2013-05-30 12:27 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Fix another signed/unsigned comparison (40.37 KB, patch)
2013-05-30 12:51 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Remove more unused code and use MediaPlayer::add*Track and remove*Track instead of calling client directly (39.98 KB, patch)
2013-05-30 13:51 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (485.43 KB, application/zip)
2013-05-30 14:43 PDT, Build Bot
no flags Details
Fix notify signal callbacks and use current-* instead of active to determine when the active pad changes (39.91 KB, patch)
2013-05-30 19:10 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
HTML file to demonstrate tracks (4.91 KB, text/html)
2013-05-31 07:53 PDT, Brendan Long
no flags Details
Fix tag handling (39.04 KB, patch)
2013-06-18 12:45 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (39.70 KB, patch)
2013-06-20 08:27 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Better headers, auto-select first audio track, and rename str to tagValue (37.80 KB, patch)
2013-06-24 16:04 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Unref pad, use static_cast, and check k if track is disconnected in event functions (39.95 KB, patch)
2013-07-25 11:05 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Change license on new files to BSD (42.09 KB, patch)
2013-07-25 11:32 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Fix unused parameter error. (42.12 KB, patch)
2013-07-25 12:58 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Add tests (1.12 MB, patch)
2013-08-22 13:55 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (800.96 KB, application/zip)
2013-08-22 23:44 PDT, Build Bot
no flags Details
Update patch based on changes to text track patch (1.16 MB, patch)
2013-08-30 16:37 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (1.16 MB, patch)
2013-10-18 12:16 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Add TrackPrivateBase base class. (26.97 KB, patch)
2013-10-28 14:22 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (1.16 MB, patch)
2013-10-30 17:29 PDT, Brendan Long
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (1.16 MB, patch)
2013-10-30 18:12 PDT, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Long 2013-05-30 11:58:50 PDT
[GStreamer] Support audio and video tracks
Comment 1 Brendan Long 2013-05-30 12:03:46 PDT
Created attachment 203371 [details]
Patch
Comment 2 Early Warning System Bot 2013-05-30 12:08:25 PDT
Comment on attachment 203371 [details]
Patch

Attachment 203371 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/714344
Comment 3 Early Warning System Bot 2013-05-30 12:11:11 PDT
Comment on attachment 203371 [details]
Patch

Attachment 203371 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/689314
Comment 4 EFL EWS Bot 2013-05-30 12:11:25 PDT
Comment on attachment 203371 [details]
Patch

Attachment 203371 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/732079
Comment 5 EFL EWS Bot 2013-05-30 12:11:32 PDT
Comment on attachment 203371 [details]
Patch

Attachment 203371 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/714348
Comment 6 Brendan Long 2013-05-30 12:12:36 PDT
This patch isn't as nice as I'd like yet, but it shows the approach I'm taking.

  * Added new callbacks to VideoTrackPrivateClient and AudioTrackPrivateClient to let it notify when the track's enabled/selected state changed, and when there's a label and language.

  * VideoTrack and AudioTrack call `m_private->set{Selected,Enabled}()` in their `set{Selected,Enabled}()` functions.

  * I split MediaPlayerPrivateGStreamer::videoChanged() into two functions: videoChanged and videoCapsChanged.

  * The {Audio,Video}TrackPrivateGStreamer classes take a pad and a playbin, and then monitor their own state (adding notify::active and notify::tags callbacks to their pad). For some reason these functions are both failing: notify::active never seems to happen, and notify::tags causes segfaults.

I think it may help to setup the video and audio tracks in the videoChanged() and audioChanged() functions, then just notify on them in notifyPlayerOfAudio() and notifyPlayerOfVideo()..
Comment 7 Brendan Long 2013-05-30 12:18:18 PDT
Created attachment 203373 [details]
Remove unnecessary old code and add #if ENABLE(VIDEO_TRACKS) when removing audio and video tracks
Comment 8 EFL EWS Bot 2013-05-30 12:22:28 PDT
Comment on attachment 203373 [details]
Remove unnecessary old code and add #if ENABLE(VIDEO_TRACKS) when removing audio and video tracks

Attachment 203373 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/680428
Comment 9 EFL EWS Bot 2013-05-30 12:24:26 PDT
Comment on attachment 203373 [details]
Remove unnecessary old code and add #if ENABLE(VIDEO_TRACKS) when removing audio and video tracks

Attachment 203373 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/689318
Comment 10 Brendan Long 2013-05-30 12:27:06 PDT
Created attachment 203374 [details]
ReFix comparison of signed and unsigned and disconnected() function
Comment 11 Brendan Long 2013-05-30 12:29:20 PDT
Oh, and it doesn't support multiple audio tracks playing at the same time because there's no good element for that yet, and I doubt I'll have time to write one.
Comment 12 EFL EWS Bot 2013-05-30 12:31:04 PDT
Comment on attachment 203374 [details]
ReFix comparison of signed and unsigned and disconnected() function

Attachment 203374 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/732083
Comment 13 EFL EWS Bot 2013-05-30 12:33:43 PDT
Comment on attachment 203374 [details]
ReFix comparison of signed and unsigned and disconnected() function

Attachment 203374 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/732084
Comment 14 Brendan Long 2013-05-30 12:45:27 PDT
... And switching audio and video tracks just causes the video to stop playing for some reason. All I do is set "current-audio" or "current-video". This doesn't happen outside of WebKit, and I think it has something to do with some recent commits for handling things asynchronously.

Which I guess means almost nothing about this patch actually works yet, but I think it's all mainly interactions with GStreamer that I'm getting wrong, so hopefully with some help this will be easy to finish.
Comment 15 Brendan Long 2013-05-30 12:51:04 PDT
Created attachment 203376 [details]
Fix another signed/unsigned comparison
Comment 16 Brendan Long 2013-05-30 13:51:52 PDT
Created attachment 203380 [details]
Remove more unused code and use MediaPlayer::add*Track and remove*Track instead of calling client directly
Comment 17 Build Bot 2013-05-30 14:43:31 PDT
Comment on attachment 203380 [details]
Remove more unused code and use MediaPlayer::add*Track and remove*Track instead of calling client directly

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

New failing tests:
editing/undo/undo-after-setting-value.html
Comment 18 Build Bot 2013-05-30 14:43:34 PDT
Created attachment 203383 [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.3
Comment 19 Brendan Long 2013-05-30 16:33:59 PDT
Add GStreamer bug[1] since input-selector's pads don't notify when "active" changes. For now I'll use current-audio and current-video.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=701319
Comment 20 Brendan Long 2013-05-30 19:10:04 PDT
Created attachment 203405 [details]
Fix notify signal callbacks and use current-* instead of active to determine when the active pad changes
Comment 21 Brendan Long 2013-05-30 19:17:29 PDT
  * No more segfaults on notify:: callbacks (my function signature was wrong).
  * Language and label work perfectly.
  * Using notify::current-audio or current-video instead of notify::active. See: https://bugzilla.gnome.org/show_bug.cgi?id=701319
  * We correctly detect when the active stream changes and set enabled/selected appropriately.
  * Selecting a different audio or video track and then seeking (by clicking on the seek bar) causes the video to stop playing. There's no errors, so I think this is something about how we're seeking in WebKit.
  * If you select a different track, it immediately cuts off audio or video, and then it takes a long time to come back. See: https://bugzilla.gnome.org/show_bug.cgi?id=680336
  * For some reason if you play a video twice, it sometimes crashes because the input-selector was deleted, but we still have a pad. I think the main problem is that we're keeping the pad around after the input-selector is gone, but I reported this too, since I don't think it should crash: https://bugzilla.gnome.org/show_bug.cgi?id=701323
Comment 22 Brendan Long 2013-05-31 07:53:09 PDT
Created attachment 203446 [details]
HTML file to demonstrate tracks

This is a test file that looks at a video with two audio and video streams and lets you switch between them. You can download the video if you need it:

https://dl.dropboxusercontent.com/u/61100892/example.mkv
Comment 23 Brendan Long 2013-06-13 10:23:37 PDT
(In reply to comment #21)
>   * [...], it sometimes crashes because the input-selector was deleted, but we still have a pad. I think the main problem is that we're keeping the pad around after the input-selector is gone [...]

The problem is that playbin doesn't emit {audio,text,video}-changed events when pads are removed. I submitted a patch for that too[1], but the workaround for older versions of GStreamer may be ugly. Two options I can think of are:

  * Check if the pad has a parent before doing anything that could crash.
  * We could only turn on tracks if GStreamer >= 1.1.1.
Comment 24 Brendan Long 2013-06-13 10:24:04 PDT
Missing footnote:

[1] https://bugzilla.gnome.org/show_bug.cgi?id=702195
Comment 25 Brendan Long 2013-06-18 12:45:07 PDT
Created attachment 204933 [details]
Fix tag handling
Comment 26 Brendan Long 2013-06-18 12:47:18 PDT
This last patch simplifies how tags are handled and makes this work on GStreamer 0.10 again, in case anyone cares about that.

The only problems I'm aware of are:

  * No tests.
  * Switching is still slow and buggy (input-selector problems).
  * Crashes the second time through unless you're on GStreamer >= 1.1.1. Fixing this on older versions may be harder than I initially thought.

For now I'm going to switch my focus back to text tracks, since no one seems interested in this.
Comment 27 Brendan Long 2013-06-18 12:48:26 PDT
Oh, and since adder pads can be muted now, we could also improve this by using an adder for audio instead of an input-selector, but I'm not sure if we care about backwards compatibility here or not.
Comment 28 Build Bot 2013-06-18 17:59:14 PDT
Comment on attachment 204933 [details]
Fix tag handling

Attachment 204933 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/904329
Comment 29 Philippe Normand 2013-06-19 01:34:50 PDT
(In reply to comment #26)
> This last patch simplifies how tags are handled and makes this work on GStreamer 0.10 again, in case anyone cares about that.
> 

I don't think it's realy important to have gst 0.10 support for new features.

> The only problems I'm aware of are:
> 
>   * No tests.
>   * Switching is still slow and buggy (input-selector problems).
>   * Crashes the second time through unless you're on GStreamer >= 1.1.1. Fixing this on older versions may be harder than I initially thought.
> 
> For now I'm going to switch my focus back to text tracks, since no one seems interested in this.

I'm interested and looking forward this code to land! I'll try to review it soon.

(In reply to comment #27)
> Oh, and since adder pads can be muted now, we could also improve this by using an adder for audio instead of an input-selector, but I'm not sure if we care about backwards compatibility here or not.

If you can make this work better using a more recent gst version I think you should go for it and enable the whole feature only for the gst version (and later) that works well.
Comment 30 Brendan Long 2013-06-19 07:42:01 PDT
(In reply to comment #29)
> If you can make this work better using a more recent gst version I think you should go for it and enable the whole feature only for the gst version (and later) that works well.

Ok, if we limit it to GStreamer 1.1.1, I can get everything working correctly except the input-selector problems (and I can look into those, it just may take me some time to figure it out).
Comment 31 Philippe Normand 2013-06-19 08:01:10 PDT
Awesome, thanks a lot Brendan!
Comment 32 Brendan Long 2013-06-20 08:27:11 PDT
Created attachment 205093 [details]
Patch
Comment 33 Brendan Long 2013-06-20 08:28:12 PDT
This patch uses `adder` for the audio tracks. There's an issue with seeking while paused: https://bugzilla.gnome.org/show_bug.cgi?id=702697

Once we have a GStreamer version that this works against, I'll put in some preprocessor checks for that version.
Comment 34 Philippe Normand 2013-06-24 07:11:52 PDT
Comment on attachment 205093 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please fix this line, remove OOPS, I suppose existing tests cover this change?

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

You can mention yourself here or the company you work for :) Unless Apple hired you? About the header below, please make sure this is really the one you want to use.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:88
> +    g_signal_handlers_disconnect_by_func(m_pad.get(),
> +        reinterpret_cast<gpointer>(audioTrackPrivateMuteChangedCallback), this);
> +    g_signal_handlers_disconnect_by_func(m_pad.get(),
> +        reinterpret_cast<gpointer>(audioTrackPrivateTagsChangedCallback), this);

Not sure indenting is needed here unless the line is wider than 120 characters. Same for various parts of the patch below.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:136
> +    gchar* str;

tagValue?

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:79
> +#endif // ENABLE(VIDEO) && USE(AVFOUNDATION) && HAVE(AVFOUNDATION_TEXT_TRACK_SUPPORT)

C&P error here I suppose :P

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:258
> +        m_audioTracks[i]->disconnect();

Hum, isn't the Vector dtor supposed to do its job here? If the disconnect method is used only here it can be merged to the Track dtor.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:261
> +        m_videoTracks[i]->disconnect();

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:606
> +        if (i < (gint)m_videoTracks.size()) {

I don't think a cast is needed here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:618
> +    while ((gint)m_videoTracks.size() > numTracks) {

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:621
> +        track->disconnect();
> +        m_videoTracks.removeLast();

Shouldn't removeLast() calls the track dtor?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:654
> +    for (gint i = 0; i < numTracks; ++i) {

Well this code is quite similar to the video tracks code above, so same questions :)

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.h:79
> +#endif // ENABLE(VIDEO) && USE(AVFOUNDATION) && HAVE(AVFOUNDATION_TEXT_TRACK_SUPPORT)

Another C&P error. The comments on AudioTrackPrivateGStreamer also apply to this module since the code is similar.
Comment 35 Brendan Long 2013-06-24 10:32:07 PDT
(In reply to comment #34)
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> Please fix this line, remove OOPS, I suppose existing tests cover this change?

Actually, I need to write the tests, since audio/video tracks don't work in any other ports yet.

> > Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:2
> > + * Copyright (C) 2013 Apple Inc. All rights reserved.
> 
> You can mention yourself here or the company you work for :) Unless Apple hired you? About the header below, please make sure this is really the one you want to use.

Oh cool. I didn't realize we were allowed to mess with that. It looks like LGPLv2 is preferred in the GStreamer sections of WebKit?

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:258
> > +        m_audioTracks[i]->disconnect();
> 
> Hum, isn't the Vector dtor supposed to do its job here? If the disconnect method is used only here it can be merged to the Track dtor.

The Vector holds RefPtr's, so the track destructor aren't necessarily called here (JavaScript could be holding a reference to a track for example).
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:606
> > +        if (i < (gint)m_videoTracks.size()) {
> 
> I don't think a cast is needed here.

EFL won't build without the cast because it's a signed/unsigned comparison.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:621
> > +        track->disconnect();
> > +        m_videoTracks.removeLast();
> 
> Shouldn't removeLast() calls the track dtor?

Same problem as above, JavaScript may still be holding a reference to the track.
Comment 36 Brendan Long 2013-06-24 16:04:50 PDT
Created attachment 205340 [details]
Better headers, auto-select first audio track, and rename str to tagValue
Comment 37 Philippe Normand 2013-07-25 03:25:48 PDT
Comment on attachment 205340 [details]
Better headers, auto-select first audio track, and rename str to tagValue

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

Looks quite good but r- mostly because of the tests issue

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Can we land this along with the new tests? It'd be really good to do so.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:130
> +    gchar* tagValue;

I think a GOwnPtr<gchar> can be used here. So no g_free call needed below.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:606
> +        g_signal_emit_by_name(m_playBin.get(), "get-video-pad", i, &pad, NULL);

I think the pad needs to be unreffed after usage. Can you please double-check it's not leaked?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:609
> +        if (i < (gint)m_videoTracks.size()) {

I'm ok with a cast but not C-style please ;)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:621
> +    while ((gint)m_videoTracks.size() > numTracks) {

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:659
> +        g_signal_emit_by_name(m_playBin.get(), "get-audio-pad", i, &pad, NULL);

Same remarks about pad unref and casts

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:133
> +    gchar* tagValue;

Same here, if a GOwnPtr<gchar> can be used it'd be better.
Comment 38 Brendan Long 2013-07-25 11:05:39 PDT
Created attachment 207468 [details]
Unref pad, use static_cast, and check k if track is disconnected in event functions

This fixes the issues above, except for the GOwnPtr<gchar> one. I also found an issue where we could get track change events even when the track is disconnected (because we get events from the input-selector), and added a check for that.
Comment 39 Brendan Long 2013-07-25 11:05:49 PDT
(In reply to comment #37)
> (From update of attachment 205340 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205340&action=review
> 
> Looks quite good but r- mostly because of the tests issue

Yes, makes sense. I've been meaning to get tests in but I've been distracted by a lot of stuff going on at work, and the in-band tracks.

I think this also can't be merged until input-selector works correctly. Right now, it takes a *long* time to switch streams.

> > Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:130
> > +    gchar* tagValue;
> 
> I think a GOwnPtr<gchar> can be used here. So no g_free call needed below.

This doesn't really simplify the code, since I still need to clear() the tagValue before I can reuse it because of GOwnPtr::outPtr():

T*& outPtr()
{
    ASSERT(!m_ptr); // <--
    return m_ptr;
}

The code would look like this:

    GOwnPtr<gchar> tagValue;
    if (gst_tag_list_get_string(tags, GST_TAG_TITLE, &tagValue.outPtr())) {
        m_label = tagValue.get();
        tagValue.clear();
        client()->audioTrackPrivateLabelChanged(this);
    }
    if (gst_tag_list_get_string(tags, GST_TAG_LANGUAGE_CODE, &tagValue.outPtr())) {
        m_language = tagValue.get();
        client()->audioTrackPrivateLanguageChanged(this);
    }

And even then we might want a clear() in the second section, so it doesn't confuse someone in the future.. I think the g_free() is simpler in this case, but it's up to you.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:606
> > +        g_signal_emit_by_name(m_playBin.get(), "get-video-pad", i, &pad, NULL);
> 
> I think the pad needs to be unreffed after usage. Can you please double-check it's not leaked?

Yes that's right. Playbin refs it before returning it. I fixed it to unref it if it's an already-used pad, or adopt it when we create a track.
Comment 40 Brendan Long 2013-07-25 11:32:03 PDT
Created attachment 207470 [details]
Change license on new files to BSD

Legal at my company wanted me to put new files under the BSD license. I assume this makes no difference at all, since MediaPlayerPrivateGStreamer is LGPL, but it's what they wanted..
Comment 41 Build Bot 2013-07-25 11:38:17 PDT
Comment on attachment 207470 [details]
Change license on new files to BSD

Attachment 207470 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1209319
Comment 42 Build Bot 2013-07-25 12:15:59 PDT
Comment on attachment 207470 [details]
Change license on new files to BSD

Attachment 207470 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1213338
Comment 43 Brendan Long 2013-07-25 12:58:43 PDT
Created attachment 207480 [details]
Fix unused parameter error.
Comment 44 Philippe Normand 2013-08-08 08:00:50 PDT
Well this looks good to me apart from the missing tests and the input-selector issue.
Comment 45 Brendan Long 2013-08-08 08:44:45 PDT
(In reply to comment #44)
> Well this looks good to me apart from the missing tests and the input-selector issue.

I'll try to get some tests done next week. I assume we don't want to commit this until input-selector works right though.
Comment 46 Brendan Long 2013-08-22 13:55:50 PDT
Created attachment 209391 [details]
Add tests

This version adds some tests (using MKV files because they're easy to work with), and fixes a bug with how I was handling the audio track tags (need to use sticky events, since adder's pads don't have the tags property). I'm not sure how to unskip these, and make sure ports that can't handle MKV files skip them..
Comment 47 Build Bot 2013-08-22 23:44:17 PDT
Comment on attachment 209391 [details]
Add tests

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

New failing tests:
media/track/audio/audio-track-mkv-vorbis-language.html
media/track/audio/audio-track-mkv-vorbis-enabled.html
media/track/video/video-track-mkv-theora-language.html
media/track/audio/audio-track-mkv-vorbis-addtrack.html
media/track/video/video-track-mkv-theora-addtrack.html
media/track/video/video-track-mkv-theora-selected.html
Comment 48 Build Bot 2013-08-22 23:44:23 PDT
Created attachment 209438 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 49 Eric Carlson 2013-08-23 10:13:23 PDT
Comment on attachment 209391 [details]
Add tests

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

> Source/WebCore/ChangeLog:32
> +        (WebCore::HTMLMediaElement::audioTrackEnabledChanged):
> +        (WebCore::HTMLMediaElement::videoTrackSelectedChanged):
> +        * html/track/AudioTrack.cpp:
> +        (WebCore::AudioTrack::setEnabled):
> +        (WebCore::AudioTrack::audioTrackPrivateEnabledChanged):
> +        (WebCore::AudioTrack::audioTrackPrivateLabelChanged):
> +        (WebCore::AudioTrack::audioTrackPrivateLanguageChanged):
> +        * html/track/AudioTrack.h:
> +        * html/track/VideoTrack.cpp:
> +        (WebCore::VideoTrack::setSelected):
> +        (WebCore::VideoTrack::videoTrackPrivateSelectedChanged):
> +        (WebCore::VideoTrack::videoTrackPrivateLabelChanged):
> +        (WebCore::VideoTrack::videoTrackPrivateLanguageChanged):

Nit: please either add comments about what a new method does or what has been changed. If it isn't worth a comment remove the entry from ChangeLog.

> LayoutTests/ChangeLog:14
> +        * media/content/two-audio-and-video-tracks.mkv: Added.
> +        * media/track/audio/audio-track-mkv-vorbis-addtrack.html: Added.
> +        * media/track/audio/audio-track-mkv-vorbis-enabled.html: Added.
> +        * media/track/audio/audio-track-mkv-vorbis-language.html: Added.
> +        * media/track/video/video-track-mkv-theora-addtrack.html: Added.
> +        * media/track/video/video-track-mkv-theora-language.html: Added.
> +        * media/track/video/video-track-mkv-theora-selected.html: Added.

Rather than making all of these format specific, could you use findMediaFile() (or something equivalent) so these basic tests can work with every port that supports the feature?
Comment 50 Brendan Long 2013-08-23 10:17:40 PDT
(In reply to comment #49)
> Rather than making all of these format specific, could you use findMediaFile() (or something equivalent) so these basic tests can work with every port that supports the feature?

There's two reasons:

  * There are several formats where we can play the audio and video, but not the subtitles (.mov is a good example). Also, how much of the spec we support depends on the format too (we get "label" for ogg files, but not for mkv files).
  * Given the above, it seems useful to test every format we support, in case something works in one format but not others.
Comment 51 Brendan Long 2013-08-23 10:20:15 PDT
(In reply to comment #50)
> (In reply to comment #49)
> > Rather than making all of these format specific, could you use findMediaFile() (or something equivalent) so these basic tests can work with every port that supports the feature?
> 
> There's two reasons:
> 
>   * There are several formats where we can play the audio and video, but not the subtitles (.mov is a good example). Also, how much of the spec we support depends on the format too (we get "label" for ogg files, but not for mkv files).
>   * Given the above, it seems useful to test every format we support, in case something works in one format but not others.

For a minute I thought I was commenting on the text track patch. For audio and video it comes down to:

  * How much of the spec we support depends on the format (we get "label" for ogg files, but not for mkv files) so, it seems useful to test every format we support, in case something works in one format but not others.

It's possible we only need one mkv file, and the internal formats don't matter. Maybe I should have made this mkv(h264 + whatever the standard audio format is)?
Comment 52 Brendan Long 2013-08-30 16:37:03 PDT
Created attachment 210176 [details]
Update patch based on changes to text track patch

I renamed the track callbacks, and combined these tests with the text track tests where appropriate (and added two more text track patches, since the code was already there for audio and video). I also changed the tags callbacks in GStreamer tracks to check all of the tag events, instead of just the first one. I still have a problem where the video will sometimes stop when we switch video tracks (I think this is an input-selector bug), so I'm not quite ready to commit this, but I wanted to get it back up-to-date.
Comment 53 Philippe Normand 2013-10-16 00:57:57 PDT
Comment on attachment 210176 [details]
Update patch based on changes to text track patch

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

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:36
> +class AudioTrackPrivateGStreamer : public AudioTrackPrivate {

I suppose this class can be FINAL

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:59
> +#ifdef GST_API_VERSION_1

This is checked already, can be removed.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:73
> +#ifdef GST_API_VERSION_1

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:640
> +        GstPad* pad;

Would it be possible to use a GRefPtr here?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:695
> +        GstPad* pad;

Ditto?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:192
> +    Vector<RefPtr<AudioTrackPrivateGStreamer> > m_audioTracks;

The space between > can be removed now.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:194
> +    Vector<RefPtr<VideoTrackPrivateGStreamer> > m_videoTracks;

Ditto

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:73
> +    g_signal_connect(m_pad.get(), "notify::tags", G_CALLBACK(videoTrackPrivateTagsChangedCallback), this);

No need for a pad probe like in AudioTrack here? Also I wonder, can some parts of both classes be factored out to a common class?

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.h:36
> +class VideoTrackPrivateGStreamer : public VideoTrackPrivate {

FINAL ?
Comment 54 Brendan Long 2013-10-16 13:31:38 PDT
(In reply to comment #53)
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:640
> > +        GstPad* pad;
> 
> Would it be possible to use a GRefPtr here?

I'd like to, but I couldn't find a way to pass a GRefPtr to g_signal_emit_by_name.

GOwnPtr has an `outPtr()` method which does what I need. I could add that to GRefPtr. That could be useful in a lot of the GStreamer code..

The other option is something like this:

        GstPad* padRawPtr;
        g_signal_emit_by_name(m_playBin.get(), "get-video-pad", i, &padRawPtr, NULL);
        ASSERT(pad);
        GRefPtr<GstPad> pad = adoptGRef(padRawPtr);

> > Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:73
> > +    g_signal_connect(m_pad.get(), "notify::tags", G_CALLBACK(videoTrackPrivateTagsChangedCallback), this);
> 
> No need for a pad probe like in AudioTrack here? Also I wonder, can some parts of both classes be factored out to a common class?

Video tracks use an input-selector, and input-selector pads have a "tag" property. adder pads don't have that property, so we need to get the tags another way. I'm not entirely sure that "adder" is the right way to handle this, but there's not really a better option right now (unless we want to just use an input-selector, but we wouldn't meet the spec..).
Comment 55 Brendan Long 2013-10-17 14:53:13 PDT
My main concern with this patch now is that adder may not be up to the task. How would you feel about just using input-selector (and only allowing one audio track to play at a time) for a start?
Comment 56 Philippe Normand 2013-10-17 23:45:03 PDT
(In reply to comment #55)
> My main concern with this patch now is that adder may not be up to the task. How would you feel about just using input-selector (and only allowing one audio track to play at a time) for a start?

That'd be a good start I think :)
Comment 57 Brendan Long 2013-10-18 12:16:02 PDT
Created attachment 214593 [details]
Patch

I updated this to use an input-selector for the audio tracks, and use GRefPtr::outPtr.
Comment 58 Brendan Long 2013-10-21 13:10:29 PDT
(In reply to comment #57)
> Created an attachment (id=214593) [details]
> Patch
> 
> I updated this to use an input-selector for the audio tracks, and use GRefPtr::outPtr.

Oh, and the changes requested by Philippe Normand.
Comment 59 Philippe Normand 2013-10-22 03:51:04 PDT
Comment on attachment 214593 [details]
Patch

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

Looks definitely better, perhaps Eric can also review this patch?

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:36
> +class AudioTrackPrivateGStreamer FINAL : public AudioTrackPrivate {

I'm still a bit worried by the similarities between AudioTrackPrivateGStreamer and the Video one. Would it be possible to share code between those two, via a mixin perhaps?
Comment 60 Brendan Long 2013-10-22 10:14:20 PDT
(In reply to comment #59)
> I'm still a bit worried by the similarities between AudioTrackPrivateGStreamer and the Video one. Would it be possible to share code between those two, via a mixin perhaps?

Because of the *Private and *PrivateClient classes, it's relatively difficult to put most of the code in a shared class. It would be really easy with generics, but it looks like they're frowned-upon in WebKit.

I should have a non-generic implementation soon though.
Comment 61 Brendan Long 2013-10-22 12:43:37 PDT
(In reply to comment #59)
> I'm still a bit worried by the similarities between AudioTrackPrivateGStreamer and the Video one. Would it be possible to share code between those two, via a mixin perhaps?

I looked into doing this with:

class TrackPrivate {};
class AudioTrackPrivate : public TrackPrivate {};
class TrackPrivateGStreamer : public TrackPrivate {};
class AudioTrackPrivateGStreamer : public AudioTrackPrivate, public TrackPrivateGstreamer {};

But apparently this leads to the "the dreaded diamond of doom"[1], which I assume is frowned up on in WebKit. I can probably make this work, but I'm not sure if it's going to be acceptable..
Comment 62 Jer Noble 2013-10-22 15:23:47 PDT
(In reply to comment #61)
> (In reply to comment #59)
> > I'm still a bit worried by the similarities between AudioTrackPrivateGStreamer and the Video one. Would it be possible to share code between those two, via a mixin perhaps?
> 
> I looked into doing this with:
> 
> class TrackPrivate {};
> class AudioTrackPrivate : public TrackPrivate {};
> class TrackPrivateGStreamer : public TrackPrivate {};
> class AudioTrackPrivateGStreamer : public AudioTrackPrivate, public TrackPrivateGstreamer {};
> 
> But apparently this leads to the "the dreaded diamond of doom"[1], which I assume is frowned up on in WebKit. I can probably make this work, but I'm not sure if it's going to be acceptable..

FWIW, we made this work in WebKit/Mac with a PIMPL that implemented the common parts of VideoTrackPrivateAVFObjC and AudioTrackPrivateAVFObjC.  See: <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.h> (which, as an aside, totally got checked into the wrong directory by yours truly).
Comment 63 Brendan Long 2013-10-28 14:22:02 PDT
Created attachment 215335 [details]
Add TrackPrivateBase base class.

This adds a TrackPrivateBase and TrackPrivateBaseClient (I'm not exactly sure about the names), so the *TrackPrivate classes can share some code. I'm looking at doing a PIMPL for the *TrackPrivateGStreamer classes to share code. I also added labelChanged() and languageChanged() to AudioTrack and VideoTrack to make them consistent. Is it easier to review if I split individual commits like this?
Comment 64 Brendan Long 2013-10-28 14:31:30 PDT
(In reply to comment #63)
> Created an attachment (id=215335) [details]
> Add TrackPrivateBase base class.

And just to make it clear: This doesn't help the *TrackPrivateGStreamer classes share anything, but it's code-reduction that I saw the potential for while I was trying to do that.
Comment 65 Brendan Long 2013-10-29 10:16:13 PDT
And I think I have a good patch that splits out shared code in the *TrackPrivateGStreamer classes, but it conflicts with the patch I just posted, so I'll wait to see what people have to say about that. If you'd prefer, I can post a version that doesn't depend on it.
Comment 66 Philippe Normand 2013-10-29 10:20:19 PDT
I haven't looked in details but if you can refactor the existing code nicely in a patch and in another patch use it, then it'd be better.

Perhaps we're actually in violent agreement here, not sure :)
Comment 67 Brendan Long 2013-10-29 10:26:35 PDT
(In reply to comment #66)
> I haven't looked in details but if you can refactor the existing code nicely in a patch and in another patch use it, then it'd be better.
> 
> Perhaps we're actually in violent agreement here, not sure :)

Yeah, so the existing patch "Add TrackPrivateBase base class." refactors AudioTrackPrivate, VideoTrackPrivate and InbandTextTrackPrivate to have a common base class (for handling id, label, language, in-band track index, and willremove() callback).

FYI, my only real concern with that patch is that I'm not really sure what to name the new class. TrackBasePrivate might match the existing style better, or even BaseTrackPrivate, to match the {whatever}TrackPrivate style..

---

My other patch is the audio and video track patch, with less code duplication, but it has minor dependencies on the first patch (I think the big one is renaming textTrackIndex(), audioTrackIndex() and videoTrackIndex() to trackIndex() so they could be moved to the base class). I'll post it after the first one gets reviewed.
Comment 68 Brendan Long 2013-10-30 17:29:56 PDT
Created attachment 215582 [details]
Patch

Here's the audio/video track patch without the dependency on the other patch. This is the one I'd really like to get in, and I can handle simplifying the *TrackPrivate classes separately.
Comment 69 EFL EWS Bot 2013-10-30 18:00:54 PDT
Comment on attachment 215582 [details]
Patch

Attachment 215582 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/17758032
Comment 70 Brendan Long 2013-10-30 18:12:11 PDT
Created attachment 215590 [details]
Patch
Comment 71 Philippe Normand 2013-11-01 02:52:12 PDT
Comment on attachment 215590 [details]
Patch

Ok, looks pretty good! BTW I couldn't find where the videoTrack RuntimeFeature is enabled, do you know?
Comment 72 Brendan Long 2013-11-01 08:37:51 PDT
(In reply to comment #71)
> (From update of attachment 215590 [details])
> Ok, looks pretty good! BTW I couldn't find where the videoTrack RuntimeFeature is enabled, do you know?

Was this supposed to be cq+ too, or are you waiting on other people to review?

The runtime checking is here:

void HTMLMediaElement::addAudioTrack(PassRefPtr<AudioTrack> track)
{
    if (!RuntimeEnabledFeatures::sharedFeatures().webkitVideoTrackEnabled())
        return;

    audioTracks()->append(track);
}

void HTMLMediaElement::addTextTrack(PassRefPtr<TextTrack> track)
{
    if (!RuntimeEnabledFeatures::sharedFeatures().webkitVideoTrackEnabled())
        return;

    textTracks()->append(track);

    closeCaptionTracksChanged();
}

void HTMLMediaElement::addVideoTrack(PassRefPtr<VideoTrack> track)
{
    if (!RuntimeEnabledFeatures::sharedFeatures().webkitVideoTrackEnabled())
        return;

    videoTracks()->append(track);
}

I'm not sure if WebKitGTK actually uses runtime enabled features though.
Comment 73 WebKit Commit Bot 2013-11-01 09:28:25 PDT
Comment on attachment 215590 [details]
Patch

Clearing flags on attachment: 215590

Committed r158436: <http://trac.webkit.org/changeset/158436>
Comment 74 Brendan Long 2013-11-01 09:34:32 PDT
Comment on attachment 215335 [details]
Add TrackPrivateBase base class.

I'll do the refactoring of the *TrackPrivate classes in a separate bug (this patch needs to be rebased anyway).