Bug 124020 – [GStreamer] Consolidate more code into TrackPrivateBaseGStreamer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brendan Long]

[GStreamer] Consolidate more code into TrackPrivateBaseGStreamer

[Brendan Long]

Created attachment 216355 [details]
Patch

This moves significantly more code into TrackPrivateBaseGStreamer, and makes it a base class for InbandTextTrackPrivateGStreamer. There's some issue with sticky tags not working correctly, but it seems to be a GStreamer issue. I'll open a bug report for that before trying to get this committed.

[Philippe Normand]

Comment on attachment 216355 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:112
>      g_signal_handlers_disconnect_by_func(m_pad.get(),

Shouldn't the handler be disconnected from playbin here?

[Brendan Long]

I moved the track initialization earlier so we can catch all of the tags, since I suspect the difference between input-selector's tags and tag events is that the input-selector merges all tags and never throws them away.

After doing that, I get output like this, which seems to confirm my theory:

Track 0 got tags: taglist, track-id=(string)12845557391394096188, language-code=(string)la, description=(string)"audiotest\ wave", encoder=(string)"Xiph.Org\ libVorbis\ I\ 20101101\ \(Schaufenugget\)", encoder-version=(uint)0, audio-codec=(string)Vorbis, nominal-bitrate=(uint)80000, bitrate=(uint)80000;.
Track 0 got tags: taglist, container-format=(string)Matroska;.

[Brendan Long]

Nevermind, I was reading my output wrong. I"m still not sure why I sometimes don't get tags.

[Brendan Long]

It turns out TrackPrivateBaseGStreamer doesn't need access to playbin at all, since I can use "notify::active" on the pads. This didn't work previously, but we need GStreamer 1.2 anyway, and the fix is in that version:

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

If "active" was set-able, this could be even better, but I think this is decent.. for now.

I opened this bug for the sticky event issue:

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

I'm planning to work around it by giving TextCombinerGStreamer its own pad type with an "active" property.

[Brendan Long]

So when I was complaining about how hard it is to extend GstGhostPad? Apparently I just needed to read the documentation. It's actually really easy:

http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstGhostPad.html#gst-ghost-pad-construct

[Brendan Long]

Created attachment 216461 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/InbandTextTrackPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:93:  webkit_text_combiner_pad_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:277:  webkit_text_combiner_pad_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 10 files


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

[Brendan Long]

What I did:

  * Added WebKitTextCombinerPad, which has a "tags" property which behaves exactly like input-selector pads' "tags" property.
  * Moved just about everything into TrackPrivateBaseGStreamer.
  * Simplified tag handling by refactoring part of it out into "getTag()". This will become more important as we add more tags (list GST_TAG_TRACK_ID and GST_TAG_TRACK_KIND hopefully).

It will fail the style check because of Glib's requirement that init functions be name type_init().

For some reason the build suddenly refuses to parallelize, so I'm submitting now, and I'll make a note after I run the tests again. The tests worked perfectly before I rebased, so I expect they will continue to.

[Brendan Long]

The build finished and the tests are passing (consistently, even!).

[Philippe Normand]

Comment on attachment 216461 [details]
Patch

Ok!

[WebKit Commit Bot]

Comment on attachment 216461 [details]
Patch

Clearing flags on attachment: 216461

Committed r159024: <http://trac.webkit.org/changeset/159024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.