Bug 77089 – [GStreamer] 0.11 support in MediaPlayerPrivateGStreamer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

SSIA

[Philippe Normand]

Created attachment 124092 [details]
0.11 MediaPlayerPrivate

[Early Warning System Bot]

Comment on attachment 124092 [details]
0.11 MediaPlayerPrivate

Attachment 124092 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11191294

[Philippe Normand]

Created attachment 124094 [details]
0.11 MediaPlayerPrivate

[Sebastian Dröge (slomo)]

Comment on attachment 124094 [details]
0.11 MediaPlayerPrivate

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:979
> +            gst_object_unref(pad);

Nope, g_value_unset() please :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1802
> +    GstElement* videoConvert = gst_element_factory_make("videoconvert", 0);

Why is this necessary in 1.0 only?

[Philippe Normand]

(In reply to comment #4)
> (From update of attachment 124094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124094&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:979
> > +            gst_object_unref(pad);
> 
> Nope, g_value_unset() please :)
> 

Ouch!

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1802
> > +    GstElement* videoConvert = gst_element_factory_make("videoconvert", 0);
> 
> Why is this necessary in 1.0 only?

Oh hum yeah I don't remember why I added that, I'll check this part again. Thanks for the review!

[Martin Robinson]

Comment on attachment 124094 [details]
0.11 MediaPlayerPrivate

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

Nice stuff.

> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:43
> +#ifdef GST_API_VERSION_1
> +        gst_object_ref_sink(GST_OBJECT(ptr));
> +#else
>          gst_object_ref(GST_OBJECT(ptr));
>          gst_object_sink(GST_OBJECT(ptr));
> +#endif
>      }

It's probably cleaner to just abstract this away into a helper function that does the right thing for each GStreamer. Imagine something like GtkVersioning.h.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:53
> +#ifdef GST_API_VERSION_1
> +#include <gst/audio/streamvolume.h>
> +#else
>  #include <gst/interfaces/streamvolume.h>
> +#endif

Headers included behind ifdefs usually go in their own block.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:78
> +#ifdef GST_API_VERSION_1
> +#define PLAYBIN_NAME "playbin"
> +#else
> +#define PLAYBIN_NAME "playbin2"
> +#endif

Please use a const char* instead of a #define.

static const char* gPlaybinName = "playbin"

[Philippe Normand]

Comment on attachment 124094 [details]
0.11 MediaPlayerPrivate

needs work

[Philippe Normand]

(In reply to comment #4)
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1802
> > +    GstElement* videoConvert = gst_element_factory_make("videoconvert", 0);
> 
> Why is this necessary in 1.0 only?

I've actually tried to remove this videoconvert element but I now get a warning from basetransform


:00:00.524129217 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:527:gst_base_transform_transform_caps:<identity> to: video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1
0:00:00.524214702 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:977:gst_base_transform_find_transform:<identity> intersecting against padtemplate ANY
0:00:00.524267991 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:1051:gst_base_transform_find_transform: have fixed caps video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1
0:00:00.524344258 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:1060:gst_base_transform_find_transform:<identity> calling fixate_caps for video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1 using caps video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1 on pad identity:src
0:00:00.524459286 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:927:gst_base_transform_default_fixate:<identity> using default caps fixate function
0:00:00.524500772 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:1065:gst_base_transform_find_transform:<identity> after fixating video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1
0:00:00.524648417 31921  0x9f8a000 DEBUG          basetransform gstbasetransform.c:1106:gst_base_transform_find_transform:<identity> FAILED to get peer of <identity:src> to accept video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1
0:00:00.524736626 31921  0x9f8a000 WARN           basetransform gstbasetransform.c:1280:gst_base_transform_setcaps:<identity> transform could not transform video/x-raw, format=(string)I420, width=(int)352, height=(int)288, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)30/1 in anything we support

If I add videoconvert some caps fields like colorimetry and chroma-site get removed from the caps before the video-sink.

[Philippe Normand]

Created attachment 128986 [details]
0.11 MediaPlayerPrivate

[Philippe Normand]

(In reply to comment #8)
> (In reply to comment #4)
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1802
> > > +    GstElement* videoConvert = gst_element_factory_make("videoconvert", 0);
> > 
> > Why is this necessary in 1.0 only?
> 
> I've actually tried to remove this videoconvert element but I now get a warning from basetransform
> 
> 

Managed to get rid of it :) I guess the issue was fixed in basetransform.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.c:26:  webkit_gst_object_ref_sink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.c:36:  webkit_gst_element_get_pad_caps is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:31:  webkit_gst_object_ref_sink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:32:  webkit_gst_element_get_pad_caps is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 6 files


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

[Alexis Menard (darktears)]

Comment on attachment 128986 [details]
0.11 MediaPlayerPrivate

Would that be possible that you patch Source/WebCore/Target.pri to compile the new files? Line 3228. It should be very trivial.

[Philippe Normand]

(In reply to comment #12)
> (From update of attachment 128986 [details])
> Would that be possible that you patch Source/WebCore/Target.pri to compile the new files? Line 3228. It should be very trivial.

Ok, I'll have a look!

[Philippe Normand]

Created attachment 128992 [details]
0.11 MediaPlayerPrivate

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.c:26:  webkit_gst_object_ref_sink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.c:36:  webkit_gst_element_get_pad_caps is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:31:  webkit_gst_object_ref_sink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:32:  webkit_gst_element_get_pad_caps is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 7 files


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

[Early Warning System Bot]

Comment on attachment 128992 [details]
0.11 MediaPlayerPrivate

Attachment 128992 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11628941

[Gyuyoung Kim]

Comment on attachment 128992 [details]
0.11 MediaPlayerPrivate

Attachment 128992 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11628956

[Philippe Normand]

(In reply to comment #16)
> (From update of attachment 128992 [details])
> Attachment 128992 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/11628941

I can't reproduce this build error here. Any help would be appreciated :)

[Philippe Normand]

Created attachment 129039 [details]
Patch

[Early Warning System Bot]

Comment on attachment 129039 [details]
Patch

Attachment 129039 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11643042

[Philippe Normand]

(In reply to comment #17)
> (From update of attachment 128992 [details])
> Attachment 128992 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/11628956

Kim can you please have a look? I can't build the EFL port here, seems like eina is too old, even on debian unstable...

[Gyuyoung Kim]

Comment on attachment 129039 [details]
Patch

Attachment 129039 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11633964

[Philippe Normand]

Created attachment 129040 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 129040 [details]
Patch

Attachment 129040 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11634006

[Philippe Normand]

Created attachment 129043 [details]
Patch

[Philippe Normand]

Crossing fingers for EFL green-go.

[Gyuyoung Kim]

Comment on attachment 129043 [details]
Patch

Attachment 129043 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11643051

[Martin Robinson]

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

r- just because a few confusing bits. Looks good generally!

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.c:22
> +#include "config.h"
> +
> +#include "GStreamerVersioning.h"

Extra newline here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:26
> +G_BEGIN_DECLS

You should not need B_BEGIN_DECLS and G_END_DECLS unless you are using this file from a C file. They just unmangle the symbols.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:978
> +            GstPad* pad = reinterpret_cast<GstPad*>(g_value_dup_object(&item));

A static_cast here should be enough.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:981
> +            if (gst_pad_query_duration(pad, fmt, &padLength)
> +                && padLength > length)

Typically lines go to 120 characters in WebKit, so don't break this line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:983
> +            g_value_unset(reinterpret_cast<GValue*>(pad));

This confuses me. You are calling g_value_unset below, why are you treating the pad like a GValue here, since it's a GstObject? Shouldn't you instead be calling gst_object_unref here? Additionally, if you used g_value_get_object instead of g_value_dup_object above, couldn't you avoid this line entirely?

[Philippe Normand]

(In reply to comment #27)
> (From update of attachment 129043 [details])
> Attachment 129043 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/11643051

This EFL build error is even more confusing.

[Philippe Normand]

Created attachment 129046 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 129046 [details]
Patch

Attachment 129046 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11639110

[Early Warning System Bot]

Comment on attachment 129046 [details]
Patch

Attachment 129046 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11641069

[Philippe Normand]

(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 129043 [details] [details])
> > Attachment 129043 [details] [details] did not pass efl-ews (efl):
> > Output: http://queues.webkit.org/results/11643051
> 
> This EFL build error is even more confusing.

Thanks Martin for the review but it'd be great to sort out this EFL issue before landing. Any EFL human-folks around?

[Philippe Normand]

Ah stupid me, I forgot to add the GStreamerVersioning files in that patch. Will send a new patch just to make sure it builds on EFL.

[Philippe Normand]

Created attachment 129047 [details]
Patch for landing

[Early Warning System Bot]

Comment on attachment 129047 [details]
Patch for landing

Attachment 129047 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11607034

[Gustavo Noronha (kov)]

Comment on attachment 129047 [details]
Patch for landing

Attachment 129047 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11644058

[Philippe Normand]

Created attachment 129051 [details]
Patch for landing, renaming GStreamerVersioning.c to GStreamerVersioning.cpp

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:36:  Declaration has space between type name and * in GstPad *pad  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:40:  Declaration has space between type name and * in GstCaps *caps  [whitespace/declaration] [3]
Total errors found: 2 in 8 files


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

[Gustavo Noronha (kov)]

Comment on attachment 129051 [details]
Patch for landing, renaming GStreamerVersioning.c to GStreamerVersioning.cpp

Attachment 129051 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11644067

[Philippe Normand]

(In reply to comment #39)
> Attachment 129051 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:36:  Declaration has space between type name and * in GstPad *pad  [whitespace/declaration] [3]
> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:40:  Declaration has space between type name and * in GstCaps *caps  [whitespace/declaration] [3]
> Total errors found: 2 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Will fix this when landing. For gtk-ews, looks like it's hosed from previous builds with the .c file, I've checked a clean build works locally.

[Philippe Normand]

Committed r109005: <http://trac.webkit.org/changeset/109005>