Bug 117039 – [GStreamer] Support audio and video tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Brendan Long]

[GStreamer] Support audio and video tracks

[Brendan Long]

Created attachment 203371 [details]
Patch

[Early Warning System Bot]

Comment on attachment 203371 [details]
Patch

Attachment 203371 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/714344

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 203371 [details]
Patch

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

[EFL EWS Bot]

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

[Brendan Long]

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

[Brendan Long]

Created attachment 203373 [details]
Remove unnecessary old code and add #if ENABLE(VIDEO_TRACKS) when removing audio and video tracks

[EFL EWS Bot]

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

[EFL EWS Bot]

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

[Brendan Long]

Created attachment 203374 [details]
ReFix comparison of signed and unsigned and disconnected() function

[Brendan Long]

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.

[EFL EWS Bot]

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

[EFL EWS Bot]

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

[Brendan Long]

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

[Brendan Long]

Created attachment 203376 [details]
Fix another signed/unsigned comparison

[Brendan Long]

Created attachment 203380 [details]
Remove more unused code and use MediaPlayer::add*Track and remove*Track instead of calling client directly

[Build Bot]

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

[Build Bot]

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

[Brendan Long]

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

[Brendan Long]

Created attachment 203405 [details]
Fix notify signal callbacks and use current-* instead of active to determine when the active pad changes

[Brendan Long]

  * 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

[Brendan Long]

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

[Brendan Long]

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

[Brendan Long]

Missing footnote:

[1] https://bugzilla.gnome.org/show_bug.cgi?id=702195

[Brendan Long]

Created attachment 204933 [details]
Fix tag handling

[Brendan Long]

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.

[Brendan Long]

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.

[Build Bot]

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

[Philippe Normand]

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

[Brendan Long]

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

[Philippe Normand]

Awesome, thanks a lot Brendan!

[Brendan Long]

Created attachment 205093 [details]
Patch

[Brendan Long]

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.

[Philippe Normand]

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.

[Brendan Long]

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

[Brendan Long]

Created attachment 205340 [details]
Better headers, auto-select first audio track, and rename str to tagValue

[Philippe Normand]

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.

[Brendan Long]

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.

[Brendan Long]

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

[Brendan Long]

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

[Build Bot]

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

[Build Bot]

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

[Brendan Long]

Created attachment 207480 [details]
Fix unused parameter error.

[Philippe Normand]

Well this looks good to me apart from the missing tests and the input-selector issue.

[Brendan Long]

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

[Brendan Long]

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

[Build Bot]

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

[Build Bot]

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

[Eric Carlson]

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?

[Brendan Long]

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

[Brendan Long]

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

[Brendan Long]

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.

[Philippe Normand]

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 ?

[Brendan Long]

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

[Brendan Long]

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?

[Philippe Normand]

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

[Brendan Long]

Created attachment 214593 [details]
Patch

I updated this to use an input-selector for the audio tracks, and use GRefPtr::outPtr.

[Brendan Long]

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

[Philippe Normand]

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?

[Brendan Long]

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

[Brendan Long]

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

[Jer Noble]

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

[Brendan Long]

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?

[Brendan Long]

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

[Brendan Long]

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.

[Philippe Normand]

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

[Brendan Long]

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

[Brendan Long]

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.

[EFL EWS Bot]

Comment on attachment 215582 [details]
Patch

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

[Brendan Long]

Created attachment 215590 [details]
Patch

[Philippe Normand]

Comment on attachment 215590 [details]
Patch

Ok, looks pretty good! BTW I couldn't find where the videoTrack RuntimeFeature is enabled, do you know?

[Brendan Long]

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

[WebKit Commit Bot]

Comment on attachment 215590 [details]
Patch

Clearing flags on attachment: 215590

Committed r158436: <http://trac.webkit.org/changeset/158436>

[Brendan Long]

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