[GStreamer] Expose MPEG-TS metadata

• Component: Media
• Status: RESOLVED FIXED

[Brendan Long]

The information is available in a GstMessage:

http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html

There is a spec from CableLabs (my company):

http://html5.cablelabs.com/tracks/media-container-mapping.html

The interesting part of this is going to be that the MPEG-TS metadata is only available if we link against gst-plugins-bad. I'm guessing WebKitGTK won't want that as a dependency, so it will probably need to be conditional.

[Brendan Long]

Actually, I mixed up -bad and -ugly. I don't think there's any particular reason to avoid linking against -bad, except that it introduce another dependency. Since we already require -base, I'm not sure if it really matters.

[Brendan Long]

Is there a better way to check if a GstMessage has MPEG-TS section data than to try to parse it? I'm currently doing this:


    GstMpegTsSection* section = gst_message_parse_mpegts_section(message);
    if (section) {
        processMpegTsSections(section);
        gst_mpegts_section_unref(section);
    }

[Philippe Normand]

Check the message type perhaps?

[Brendan Long]

(In reply to comment #3)
> Check the message type perhaps?

As far as I can tell, there's no MPEG-related message types:

http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstMessage.html#GstMessageType
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html

I'm guessing it's GST_MESSAGE_TYPE_UNKNOWN, right?

[Brendan Long]

Or maybe it's a GST_MESSAGE_ELEMENT?

[Philippe Normand]

Make sure with the GST_MESSAGE_TYPE_NAME macro :)

[Brendan Long]

I'm also having trouble getting it to link:

gcc $(gst-git pkg-config --cflags --libs gstreamer-plugins-bad-1.0) test.c 
/tmp/ccyU76G0.o:test.c:function main: error: undefined reference to 'gst_message_parse_mpegts_section'
collect2: error: ld returned 1 exit status

[Brendan Long]

Created this GStreamer bug for the linking issue:

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

[Brendan Long]

Created attachment 213128 [details]
Initial patch, needs tests

Here's an initial patch. I need to make it not break the build when gstreamer-mpegts-1.0 isn't available, and it needs some tests (the file I've been using is much too large to check in). I have a question for people with more experience with WebKit: Is there a better way to generate JSON? JSONObject appears to be designed for JavaScriptCore, and I didn't see anything else JSON related, but WebKit is a big place so I might have missed it.

[Early Warning System Bot]

Comment on attachment 213128 [details]
Initial patch, needs tests

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

[Early Warning System Bot]

Comment on attachment 213128 [details]
Initial patch, needs tests

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

[EFL EWS Bot]

Comment on attachment 213128 [details]
Initial patch, needs tests

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

[EFL EWS Bot]

Comment on attachment 213128 [details]
Initial patch, needs tests

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

[Brendan Long]

For making this compile on older versions, it's possible to check GST_CHECK_VERSION, but I'm not sure how to remove that requirement from pkg-config:

        PKGCONFIG += glib-2.0 gio-2.0 gstreamer-1.0 gstreamer-app-1.0 gstreamer-base-1.0 gstreamer-mpegts-1.0 gstreamer-pbutils-1.0 gstreamer-plugins-base-1.0 gstreamer-video-1.0 gstreamer-audio-1.0

I'll look into this more tomorrow, but if anyone knows off the top of their head, it would be helpful.

[kov's GTK+ EWS bot]

Comment on attachment 213128 [details]
Initial patch, needs tests

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

[Brendan Long]

Created attachment 213170 [details]
Make gstreamer-mpegts optional.

I'm not really sure how to handle this in WebKitGTK or EFLWebKit. I think I have an idea of how to do the GTK version (another PKG_CHECK_MODULES check with AC_DEFINE([WTF_USE_GSTREAMER_MPEGTS]), and somehow concatenate GSTREAMER_MPEGTS_LIBS and GSTREAMER_LIBS, etc. I need to get WebKitGTK building so I can test that. For CMake, I'm not even sure where the libraries are being found yet.

[Eric Carlson]

(In reply to comment #0)
> The information is available in a GstMessage:
> 
> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html
> 
> There is a spec from CableLabs (my company):
> 
> http://html5.cablelabs.com/tracks/media-container-mapping.html
> 
> The interesting part of this is going to be that the MPEG-TS metadata is only available if we link against gst-plugins-bad. I'm guessing WebKitGTK won't want that as a dependency, so it will probably need to be conditional.

I don't think it makes sense to do this until we update our TextTrack implementation to the current spec (bug 122218), which (among other things) has added a DataCue interface specifically for in-band metadata tracks.

[Brendan Long]

(In reply to comment #17)
> I don't think it makes sense to do this until we update our TextTrack implementation to the current spec (bug 122218), which (among other things) has added a DataCue interface specifically for in-band metadata tracks.

I'm not sure we want to present this as binary though. For one thing, it would be significantly harder for JS developers to use. For another, GStreamer isn't really designed to give out binary data. I guess I should see what the W3C says about this spec..

[Eric Carlson]

(In reply to comment #18)
> (In reply to comment #17)
> > I don't think it makes sense to do this until we update our TextTrack implementation to the current spec (bug 122218), which (among other things) has added a DataCue interface specifically for in-band metadata tracks.
> 
> I'm not sure we want to present this as binary though. For one thing, it would be significantly harder for JS developers to use. For another, GStreamer isn't really designed to give out binary data. I guess I should see what the W3C says about this spec..

I don't think DataCue is just for binary data, the spec says "if the browser is unable to identify a TextTrackCue interface that is more appropriate to expose the data in the cues of a media-resource-specific text track, the DataCue object is used." 

Or does "MPEG-TS metadata" actually mean *captions* in the MPEG transport stream?

[1] http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#text-tracks-exposing-in-band-metadata

[Brendan Long]

(In reply to comment #19)
> I don't think DataCue is just for binary data, the spec says "if the browser is unable to identify a TextTrackCue interface that is more appropriate to expose the data in the cues of a media-resource-specific text track, the DataCue object is used."

What's strange about using a DataCue is that the .data attribute doesn't make much sense in this case, since we can either expose exactly the same data as .text, but as binary, or we can expose the binary PMT, which would be relatively difficult and not very useful.

> Or does "MPEG-TS metadata" actually mean *captions* in the MPEG transport stream?

No, captions will be handled by the existing in-band track code (once our decoder gets into GStreamer).

[Glenn Adams]

(In reply to comment #0)
> The information is available in a GstMessage:
> 
> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html
> 
> There is a spec from CableLabs (my company):
> 
> http://html5.cablelabs.com/tracks/media-container-mapping.html
> 
> The interesting part of this is going to be that the MPEG-TS metadata is only available if we link against gst-plugins-bad. I'm guessing WebKitGTK won't want that as a dependency, so it will probably need to be conditional.

I think it better to expose via the binary, ArrayBuffer using the data attribute. The only reason the MPEG-2 mapping spec used the text attribute (to expose binary data as BASE64) is because there was no attribute to expose binary data.

The correct solution would be to update the MPEG-2 mapping spec to use the new DataCue interface and expose as binary directly. However, for compatibility with the previous mapping, one might *also* expose it as BASE64 via the text attribute.

[Philippe Normand]

So, does this patch needs a review or does it need to be reworked addressing Eric's and Glenn's doubts?
It'd be nice to get it working in the GTK+ port too at least, now that Qt was removed from trunk.

[Brendan Long]

The patch will work in GTK as soon as I can figure out how to add an optional pkgconfig dependency.

I was planning to get back to that after refactoring the cue interfaces to match the new spec. I'm starting to wonder if binary is a useful format for this though, since at least in GStreamer, it's much easier to get structured data than the original binary.

[Philippe Normand]

Comment on attachment 213170 [details]
Make gstreamer-mpegts optional.

Ok so I r- this for now :)

[Brendan Long]

Created attachment 224018 [details]
Patch

[Brendan Long]

This latest patch is a work-in-progress using DataCue. I think the only remaining issue is that I'm using GstMpegTsSection->data and GstMpegTsSection->section_length, which are private API. I need to either convince GStreamer to make those public, or rebuild the binary PMT by hand, which doesn't sound very fun.

Do we have any WebKit-y way to write bitstreams?

[Brendan Long]

I'm attempting to add a function to GStreamer to get at this data (gst_mpegts_section_get_data()):

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

[Brendan Long]

I'll also need to add a version check once this is finished, so we can make sure GStreamer provides the function.

[Eric Carlson]

Comment on attachment 224018 [details]
Patch

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

Don't we also need inBandMetadataTrackDispatchType?

> Source/WebCore/html/track/InbandDataTextTrack.h:53
> +    virtual void removeCue(TextTrackCue*, ExceptionCode&) override { ASSERT_NOT_REACHED(); }

Why should this never be called, can data cues not be removed?

> Source/WebCore/html/track/InbandWebVTTTextTrack.h:56
> +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const char*, unsigned) override { ASSERT_NOT_REACHED(); }
>      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(); }

Instead of requiring each derived class to implement ASSERTing versions of the methods they don't need, why not make the base class versions ASSERT instead of being pure virtual?

[Brendan Long]

Created attachment 224075 [details]
Patch

Moved InbandTextTrackPrivateClient ASSERTing functions to InbandTextTrack, and added InbandDataTextTrack to the Mac build to hopefully fix that linking error. For the WebKitGTK error, I'll probably change the USE(GSTREAMER_MPEG) check to check the GStreamer version, instead of blindly relying on gstreamer-mpegts-1.0 existing.

[Brendan Long]

I noticed after uploading that InbandDataTextTrack.cpp has a bunch of unnecessary #includes. The next patch I upload will have those removed.

[Brendan Long]

Created attachment 224079 [details]
Patch

Removed unused parameters (caused Mac build errors) and #includes.

[Eric Carlson]

Comment on attachment 224079 [details]
Patch

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

Don't we also need textTrack.inBandMetadataTrackDispatchType?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1124
> +        m_trackDescriptionTrack = InbandMetadataTextTrackPrivateGStreamer::create(InbandTextTrackPrivate::Metadata,
> +            InbandTextTrackPrivate::Data, "video/mp2t track-description");

Do you really want this hardcoded label on all metadata tracks?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1140
> +    m_chaptersTrack = InbandMetadataTextTrackPrivateGStreamer::create(InbandTextTrackPrivate::Chapters, InbandTextTrackPrivate::Generic);

Are chapter tracks never labeled?

[Brendan Long]

(In reply to comment #33)
> (From update of attachment 224079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224079&action=review
> 
> Don't we also need textTrack.inBandMetadataTrackDispatchType?

I just looked at the HTML5 spec for this, and it sounds like this was written before DataCue existed, since it wants us to encode binary as base64:

http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#steps-to-expose-a-media-resource-specific-text-track

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1124
> > +        m_trackDescriptionTrack = InbandMetadataTextTrackPrivateGStreamer::create(InbandTextTrackPrivate::Metadata,
> > +            InbandTextTrackPrivate::Data, "video/mp2t track-description");
> 
> Do you really want this hardcoded label on all metadata tracks?

Only for MPEG-TS PMT tracks. I just looked at our spec again, and it's actually supposed to be the "id" though.

> Are chapter tracks never labeled?

I don't know what I would use for the label.

[Brendan Long]

Created attachment 224797 [details]
Patch

[Brendan Long]

This now relies on public API (to be released in GStreamer 1.3.1). I also switched the 'video/mp2t track-description' to be the id, not the label.

[Philippe Normand]

Comment on attachment 224797 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:58
> +extern "C" {
> +#include <gst/mpegts/mpegts.h>
> +}

Having to use explicit extern "C" here is odd. Have you filed a gstreamer bug for this?

> Source/autotools/FindDependencies.m4:421
> +    PKG_CHECK_MODULES([GSTREAMER_MPEGTS], [gstreamer-mpegts-1.0 >= 1.3.0],

I think the cmake build needs a similar change

[Brendan Long]

Created attachment 224935 [details]
Patch

It looks like the extern C{} was unnecessary. I looked at CMake build support, but I wasn't able to get it working. I'm about to be out of the office for two weeks, but I'll look into the CMake build more when I get back.

[Brendan Long]

It sounds like the working group is planning to use the inbandMetadataTrackDispatch type for this instead:

http://www.w3.org/TR/html5/embedded-content-0.html#steps-to-expose-a-media-resource-specific-text-track

I'll rework my patch to use that.

[Brendan Long]

I figured out why my patch wasn't working. The CMake build wasn't finding GStreamer libraries when built from gst-uninstalled:

https://bugs.webkit.org/show_bug.cgi?id=130418

I'm waiting on the W3C to finalize some things before moving forward with a new inbandMetadataWhatever patch though.

[Eric Carlson]

(In reply to comment #40)
> 
> I'm waiting on the W3C to finalize some things before moving forward with a new inbandMetadataWhatever patch though.

What issues are you waiting on? 

If they aren't *too* disruptive, I think don't think it would hurt to go ahead and get this in now and get some experience with it. We can update once the issue(s) have been settled without worrying about existing content.

[Brendan Long]

(In reply to comment #41)
> (In reply to comment #40)
> > 
> > I'm waiting on the W3C to finalize some things before moving forward with a new inbandMetadataWhatever patch though.
> 
> What issues are you waiting on? 
> 
> If they aren't *too* disruptive, I think don't think it would hurt to go ahead and get this in now and get some experience with it. We can update once the issue(s) have been settled without worrying about existing content.

There were a lot of proposals flying around for metadata recently:

  * Making inbandMetadataBlahBlah available for all tracks.
  * Splitting that attribute with the unreasonable long name into two attributes, one ArrayBuffer with the binary metadata and one DOMString to tell you what format it is.
  * Using DataCues for the PMT like I was originally planning to implement.

It seems likely that inbandMetadata[...] whatever will likely be the final version, so I can implement that now if you're interested. I just didn't want to waste your time by applying a patch that might be need to be removed or substantially changed.

[Eric Carlson]

(In reply to comment #42)
> 
> There were a lot of proposals flying around for metadata recently:
> 
>   * Making inbandMetadataBlahBlah available for all tracks.
>   * Splitting that attribute with the unreasonable long name into two attributes, one ArrayBuffer with the binary metadata and one DOMString to tell you what format it is.
>   * Using DataCues for the PMT like I was originally planning to implement.
> 
> It seems likely that inbandMetadata[...] whatever will likely be the final version, so I can implement that now if you're interested. I just didn't want to waste your time by applying a patch that might be need to be removed or substantially changed.

I think we should go ahead. As you say, we can change it when/if necessary.

[Brendan Long]

I've had a bunch going on but I'm getting back to this.

Should inBandMetadataTrackDispatchType be an AtomicString or String? It seems unlikely that people would be checking "if (track.inBandMetadataTrackDispatchType == ...)", and internally it doesn't help us much since generating the value is 90% of the work. Everything else is an AtomicString though.

[Eric Carlson]

(In reply to comment #44)
> I've had a bunch going on but I'm getting back to this.
> 
> Should inBandMetadataTrackDispatchType be an AtomicString or String? It seems unlikely that people would be checking "if (track.inBandMetadataTrackDispatchType == ...)", and internally it doesn't help us much since generating the value is 90% of the work. Everything else is an AtomicString though.

I don't think it will matter a great either way.

[Brendan Long]

Created attachment 228446 [details]
Patch

[Brendan Long]

I think this is correct. I need to check with someone to make sure the inbandMetadataTrackDispatchType is correct (I'm pretty sure I'm doing exactly what the spec says to do, but I need to do a sanity test on the output, which requires figuring out what these values actually mean).

[Eric Carlson]

Comment on attachment 228446 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1072
> +        track->client()->addDataCue(track.get(), currentTimeDouble(), currentTimeDouble(), bytes, size);

Nit: it would be cleaner to have track->addDataCue() so you don't have to call the client interface directly.

[Xabier Rodríguez Calvar]

Comment on attachment 228446 [details]
Patch

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

This is my an informal review as I am not a reviewer. Feel free to cherry pick what you think is best, considering that eric has already r+ it.

> Source/WebCore/PlatformGTK.cmake:499
> +
>      Modules/quota/StorageInfo.idl
>      Modules/quota/StorageQuota.idl
> +

Nit: why are you adding these two lines?

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:138
> +

Nit: not necessary line?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:54
> +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS)

This is only a though coming to my mind rather than a recommendation: When I see these #if statements I always wonder if it is better to just ensure that that GSTREAMER_MPEGTS is only enabled with VIDEO_TRACK is also enabled, then you could avoid asking only for GSTREAMER_MPEGTS. I can see reasons to keep it like this so feel free to completely ignore this.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:989
> +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS)
> +        else if ((section = gst_message_parse_mpegts_section(message))) {
> +            processMpegTsSection(section);
> +            gst_mpegts_section_unref(section);
> +        }
> +#endif

Personal preference: if I were you, I'd open the else, declare the variable inside while retrieving the value (this way you can remove the declaration from line 879) and then check for the content, operate and free it.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1032
> +        for (guint i = 0; i < pmt->streams->len; ++i) {

unsigned instead of guint?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1034
> +            if (stream->stream_type == 0x05 || stream->stream_type >= 0x80) {

Personal preference: what about adding constants for these values? I guess retrieving them from any other header file is impossible or very painful, right?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1051
> +                for (guint j = 0; j < stream->descriptors->len; ++j) {

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1053
> +                    for (guint k = 0; k < descriptor->length; ++k)

Ditto.

[Brendan Long]

(In reply to comment #49)
> > Source/WebCore/PlatformGTK.cmake:499
> > +
> >      Modules/quota/StorageInfo.idl
> >      Modules/quota/StorageQuota.idl
> > +
> 
> Nit: why are you adding these two lines?

The style checker was complaining about them (even though they're not lines I changed?), so I just added the two newlines so it would pass the style check 100%.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:54
> > +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS)
> 
> This is only a though coming to my mind rather than a recommendation: When I see these #if statements I always wonder if it is better to just ensure that that GSTREAMER_MPEGTS is only enabled with VIDEO_TRACK is also enabled, then you could avoid asking only for GSTREAMER_MPEGTS. I can see reasons to keep it like this so feel free to completely ignore this.

At some point I expect this to solve itself, when ENABLE_VIDEO_TRACK is removed (I think all ports turn it on right now). Personally, I prefer to have both checks, but I don't have strong feelings either way.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:989
> > +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS)
> > +        else if ((section = gst_message_parse_mpegts_section(message))) {
> > +            processMpegTsSection(section);
> > +            gst_mpegts_section_unref(section);
> > +        }
> > +#endif
> 
> Personal preference: if I were you, I'd open the else, declare the variable inside while retrieving the value (this way you can remove the declaration from line 879) and then check for the content, operate and free it.

That makes sense. I'll make this change.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1032
> > +        for (guint i = 0; i < pmt->streams->len; ++i) {
> 
> unsigned instead of guint?

streams is a GPtrArray and len is a guint. unsigned in probably the same type, but it seemed better to be exact.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1034
> > +            if (stream->stream_type == 0x05 || stream->stream_type >= 0x80) {
> 
> Personal preference: what about adding constants for these values? I guess retrieving them from any other header file is impossible or very painful, right?

The spec I was following gives these as hex values:

http://www.cablelabs.com/wp-content/uploads/specdocs/CL-SP-HTML5-MAP-I03-140207.pdf

I could replace 0x05 with a constant, but for the '>=' comparison it seems like using a named constant would just be confusing. I'm actually checking with someone internally to make sure these values are correct, since 0x80 - 0x87 are apparently audio :\

http://en.wikipedia.org/wiki/Program_Specific_Information#Elementary_stream_types

[Xabier Rodríguez Calvar]

(In reply to comment #50)
> (In reply to comment #49)
> > > Source/WebCore/PlatformGTK.cmake:499
> > > +
> > >      Modules/quota/StorageInfo.idl
> > >      Modules/quota/StorageQuota.idl
> > > +
> > 
> > Nit: why are you adding these two lines?
> 
> The style checker was complaining about them (even though they're not lines I changed?), so I just added the two newlines so it would pass the style check 100%.

For me it looks like a bug in the style checker, specially if you haven't changed the lines. I'd go without the lines and file a bug against the style checker.

> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1032
> > > +        for (guint i = 0; i < pmt->streams->len; ++i) {
> > 
> > unsigned instead of guint?
> 
> streams is a GPtrArray and len is a guint. unsigned in probably the same type, but it seemed better to be exact.

In out code you can find inconsistencies like this for both sides so maybe Phil has an opinion about what to do for newly written code. If I pointed this out is because I remember some comments in this direction even when mixing WebKit and GStreamer code.

[Brendan Long]

Created attachment 228516 [details]
Patch

[Brendan Long]

I made some changes:

  * Added addDataCue() and addGenericCue() to InbandMetadataTextTrackPrivateGStreamer.
  * Moved the GstMpegTsSection* declaration in MediaPlayerPrivateGStreamer::handleMessage() closer to where it's used.
  * Fixed the gstreamer-mpegts check to require >=1.3.0 to hopefully fix the gtk-wk2 build.
  * Added the bits to make this work in the Efl build too (OptionsEfl.cmake).

I also checked with people to make sure that the 0x05 and 0x80 values are correct. I don't think it's any less confusing with named constants though :\

I'm planning to commit this if it builds properly, assuming I get another r+ (so the build bot won't complain about the "Oops no reviewer" line).

[Eric Carlson]

Comment on attachment 228516 [details]
Patch

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

> Source/WebCore/html/track/InbandDataTextTrack.h:48
> +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override;

Nit: please name the "start" and "end" variables.

> Source/WebCore/html/track/InbandTextTrack.h:65
> +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override { ASSERT_NOT_REACHED(); }

Ditto.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:137
> +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) = 0;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/InbandMetadataTextTrackPrivateGStreamer.h:50
> +    void addDataCue(double start, double stop, const void* data, unsigned length)

"stop" - do you mean "end"?

[Eric Carlson]

Comment on attachment 228516 [details]
Patch

Lets hold off on this for a few days.

[Eric Carlson]

Comment on attachment 228516 [details]
Patch

(In reply to comment #55)
> (From update of attachment 228516 [details])
> Lets hold off on this for a few days.

Actually, lets go ahead and get this checked in. We talked about a potential change to DataCue at the face-to-face today and having this in will make it simpler to experiment with the change.

[Brendan Long]

(In reply to comment #54)
> (From update of attachment 228516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228516&action=review
> 
> > Source/WebCore/html/track/InbandDataTextTrack.h:48
> > +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override;
> 
> Nit: please name the "start" and "end" variables.
> 
> > Source/WebCore/html/track/InbandTextTrack.h:65
> > +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override { ASSERT_NOT_REACHED(); }
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:137
> > +    virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) = 0;
> 
> Ditto.

Should I name the data and length parameters too?

[Brendan Long]

Created attachment 228966 [details]
Patch

I added parameter names where I could (in InbandTextTrack.h, it complains about unused parameters if we name them).

[WebKit Commit Bot]

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


ERROR: Source/WebCore/PlatformGTK.cmake:346:  There should be exactly one empty line instead of 0 between "Modules/gamepad/GamepadList.idl" and "Modules/geolocation/Geolocation.idl".  [list/emptyline] [5]
ERROR: Source/WebCore/PlatformGTK.cmake:347:  There should be exactly one empty line instead of 0 between "Modules/geolocation/Geolocation.idl" and "Modules/mediasource/VideoPlaybackQuality.idl".  [list/emptyline] [5]
Total errors found: 2 in 26 files


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

[Eric Carlson]

(In reply to comment #57)
> 
> Should I name the data and length parameters too?

I don't think that is necessary. I only suggested naming start and end because both are doubles.

[Brendan Long]

Created attachment 228967 [details]
Patch

More style fixes. I don't know why it complains about the entire .cmake file when you change one line, but might as well fix these while I'm here. I also removed the data and length parameter names in function definitions.

[WebKit Commit Bot]

Comment on attachment 228967 [details]
Patch

Clearing flags on attachment: 228967

Committed r167025: <http://trac.webkit.org/changeset/167025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.