Summary: | [GStreamer] 0.11 support in MediaPlayerPrivateGStreamer | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Philippe Normand <pnormand> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | gustavo, menard, mrobinson, pnormand, rakuco, slomo, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 77005, 77087 | ||||||||||||||||||||||||
Attachments: |
|
Description
Philippe Normand
2012-01-26 03:00:15 PST
Created attachment 124092 [details]
0.11 MediaPlayerPrivate
Comment on attachment 124092 [details] 0.11 MediaPlayerPrivate Attachment 124092 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11191294 Created attachment 124094 [details]
0.11 MediaPlayerPrivate
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? (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 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 on attachment 124094 [details]
0.11 MediaPlayerPrivate
needs work
(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. Created attachment 128986 [details]
0.11 MediaPlayerPrivate
(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. 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 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.
(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! Created attachment 128992 [details]
0.11 MediaPlayerPrivate
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 on attachment 128992 [details] 0.11 MediaPlayerPrivate Attachment 128992 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11628941 Comment on attachment 128992 [details] 0.11 MediaPlayerPrivate Attachment 128992 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11628956 (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 :) Created attachment 129039 [details]
Patch
Comment on attachment 129039 [details] Patch Attachment 129039 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11643042 (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 on attachment 129039 [details] Patch Attachment 129039 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11633964 Created attachment 129040 [details]
Patch
Comment on attachment 129040 [details] Patch Attachment 129040 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11634006 Created attachment 129043 [details]
Patch
Crossing fingers for EFL green-go. Comment on attachment 129043 [details] Patch Attachment 129043 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11643051 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? (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. Created attachment 129046 [details]
Patch
Comment on attachment 129046 [details] Patch Attachment 129046 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11639110 Comment on attachment 129046 [details] Patch Attachment 129046 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11641069 (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? 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. Created attachment 129047 [details]
Patch for landing
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 on attachment 129047 [details] Patch for landing Attachment 129047 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11644058 Created attachment 129051 [details]
Patch for landing, renaming GStreamerVersioning.c to GStreamerVersioning.cpp
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 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 (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. Committed r109005: <http://trac.webkit.org/changeset/109005> |