Bug 77089 - [GStreamer] 0.11 support in MediaPlayerPrivateGStreamer
Summary: [GStreamer] 0.11 support in MediaPlayerPrivateGStreamer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks: 77005 77087
  Show dependency treegraph
 
Reported: 2012-01-26 03:00 PST by Philippe Normand
Modified: 2012-02-27 10:04 PST (History)
8 users (show)

See Also:


Attachments
0.11 MediaPlayerPrivate (10.46 KB, patch)
2012-01-26 03:16 PST, Philippe Normand
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
0.11 MediaPlayerPrivate (10.91 KB, patch)
2012-01-26 03:31 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
0.11 MediaPlayerPrivate (17.25 KB, patch)
2012-02-27 01:57 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
0.11 MediaPlayerPrivate (18.40 KB, patch)
2012-02-27 02:26 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (17.99 KB, patch)
2012-02-27 08:14 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (17.87 KB, patch)
2012-02-27 08:21 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (15.21 KB, patch)
2012-02-27 08:31 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (15.42 KB, patch)
2012-02-27 08:55 PST, Philippe Normand
mrobinson: review+
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (19.09 KB, patch)
2012-02-27 09:04 PST, Philippe Normand
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch for landing, renaming GStreamerVersioning.c to GStreamerVersioning.cpp (19.16 KB, patch)
2012-02-27 09:18 PST, Philippe Normand
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-01-26 03:00:15 PST
SSIA
Comment 1 Philippe Normand 2012-01-26 03:16:13 PST
Created attachment 124092 [details]
0.11 MediaPlayerPrivate
Comment 2 Early Warning System Bot 2012-01-26 03:21:22 PST
Comment on attachment 124092 [details]
0.11 MediaPlayerPrivate

Attachment 124092 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11191294
Comment 3 Philippe Normand 2012-01-26 03:31:18 PST
Created attachment 124094 [details]
0.11 MediaPlayerPrivate
Comment 4 Sebastian Dröge (slomo) 2012-01-30 02:45:08 PST
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?
Comment 5 Philippe Normand 2012-01-30 03:12:49 PST
(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!
Comment 6 Martin Robinson 2012-01-30 08:47:51 PST
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"
Comment 7 Philippe Normand 2012-02-15 06:31:03 PST
Comment on attachment 124094 [details]
0.11 MediaPlayerPrivate

needs work
Comment 8 Philippe Normand 2012-02-15 07:10:40 PST
(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.
Comment 9 Philippe Normand 2012-02-27 01:57:12 PST
Created attachment 128986 [details]
0.11 MediaPlayerPrivate
Comment 10 Philippe Normand 2012-02-27 01:58:58 PST
(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.
Comment 11 WebKit Review Bot 2012-02-27 02:02:26 PST
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.
Comment 12 Alexis Menard (darktears) 2012-02-27 02:14:17 PST
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.
Comment 13 Philippe Normand 2012-02-27 02:20:09 PST
(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!
Comment 14 Philippe Normand 2012-02-27 02:26:32 PST
Created attachment 128992 [details]
0.11 MediaPlayerPrivate
Comment 15 WebKit Review Bot 2012-02-27 02:30:11 PST
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.
Comment 16 Early Warning System Bot 2012-02-27 03:00:26 PST
Comment on attachment 128992 [details]
0.11 MediaPlayerPrivate

Attachment 128992 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11628941
Comment 17 Gyuyoung Kim 2012-02-27 03:32:57 PST
Comment on attachment 128992 [details]
0.11 MediaPlayerPrivate

Attachment 128992 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11628956
Comment 18 Philippe Normand 2012-02-27 07:55:54 PST
(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 :)
Comment 19 Philippe Normand 2012-02-27 08:14:27 PST
Created attachment 129039 [details]
Patch
Comment 20 Early Warning System Bot 2012-02-27 08:19:27 PST
Comment on attachment 129039 [details]
Patch

Attachment 129039 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11643042
Comment 21 Philippe Normand 2012-02-27 08:19:40 PST
(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...
Comment 22 Gyuyoung Kim 2012-02-27 08:20:27 PST
Comment on attachment 129039 [details]
Patch

Attachment 129039 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11633964
Comment 23 Philippe Normand 2012-02-27 08:21:03 PST
Created attachment 129040 [details]
Patch
Comment 24 Gyuyoung Kim 2012-02-27 08:26:16 PST
Comment on attachment 129040 [details]
Patch

Attachment 129040 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11634006
Comment 25 Philippe Normand 2012-02-27 08:31:10 PST
Created attachment 129043 [details]
Patch
Comment 26 Philippe Normand 2012-02-27 08:33:46 PST
Crossing fingers for EFL green-go.
Comment 27 Gyuyoung Kim 2012-02-27 08:36:46 PST
Comment on attachment 129043 [details]
Patch

Attachment 129043 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11643051
Comment 28 Martin Robinson 2012-02-27 08:37:33 PST
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?
Comment 29 Philippe Normand 2012-02-27 08:40:51 PST
(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.
Comment 30 Philippe Normand 2012-02-27 08:55:03 PST
Created attachment 129046 [details]
Patch
Comment 31 Gyuyoung Kim 2012-02-27 09:00:16 PST
Comment on attachment 129046 [details]
Patch

Attachment 129046 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11639110
Comment 32 Early Warning System Bot 2012-02-27 09:01:21 PST
Comment on attachment 129046 [details]
Patch

Attachment 129046 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11641069
Comment 33 Philippe Normand 2012-02-27 09:02:14 PST
(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?
Comment 34 Philippe Normand 2012-02-27 09:03:57 PST
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.
Comment 35 Philippe Normand 2012-02-27 09:04:43 PST
Created attachment 129047 [details]
Patch for landing
Comment 36 Early Warning System Bot 2012-02-27 09:09:49 PST
Comment on attachment 129047 [details]
Patch for landing

Attachment 129047 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11607034
Comment 37 Gustavo Noronha (kov) 2012-02-27 09:14:22 PST
Comment on attachment 129047 [details]
Patch for landing

Attachment 129047 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11644058
Comment 38 Philippe Normand 2012-02-27 09:18:34 PST
Created attachment 129051 [details]
Patch for landing, renaming GStreamerVersioning.c to GStreamerVersioning.cpp
Comment 39 WebKit Review Bot 2012-02-27 09:21:01 PST
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.
Comment 40 Gustavo Noronha (kov) 2012-02-27 09:23:08 PST
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
Comment 41 Philippe Normand 2012-02-27 09:43:35 PST
(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.
Comment 42 Philippe Normand 2012-02-27 10:04:43 PST
Committed r109005: <http://trac.webkit.org/changeset/109005>