Summary: | [GStreamer] MediaPlayer's code is not easily reusable by other GStreamer-based players | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Danilo de Paula <danilo.eu> | ||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric.carlson, feature-media-reviews, gtk-ews, gustavo, gyuyoung.kim, hta, jonathon, menard, mrobinson, pnormand, rakuco, s.choi, tommyw, webkit.review.bot, xan.lopez, zan | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 87514 | ||||||||||||||
Attachments: |
|
Description
Danilo de Paula
2012-10-24 09:08:15 PDT
Created attachment 170410 [details]
core patch
Attachment 170410 [details] did not pass style-queue:
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:37: Missing space before { [whitespace/braces] [5]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:124: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:125: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:126: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:127: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:128: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:129: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:131: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:132: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:133: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:134: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:135: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:136: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:137: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:138: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:139: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:141: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:414: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:618: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:92: Declaration has space between type name and * in MediaStreamComponent *component [whitespace/declaration] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:93: Declaration has space between type name and * in MediaStreamSource *source [wFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.am', u'Source/W..." exit_code: 1
hitespace/declaration] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:100: Declaration has space between type name and * in MediaStreamComponent *component [whitespace/declaration] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:101: Declaration has space between type name and * in MediaStreamSource *source [whitespace/declaration] [3]
Total errors found: 25 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Do you plan to factor the copy-pasted code from the mediaplayer to some common module? (In reply to comment #3) > Do you plan to factor the copy-pasted code from the mediaplayer to some common module? philippe: actually I think that StreamMediaPlayer and the regular MediaPlayer could be merged in a near future. But I want to do it in a safe way. My idea is to push it as a different player now, keep a bug to reminds me/us to remove it in the near future. The thing is that I don't want to mess up with the current working mediaplayer without testing StreamMediaPlayer pieces in the real world. What do you think about that? We have buildbots reasonably testing the current player. If possible it'd be better to merge both players now already, in my opinion. Or minimize the size of that new player, in that state i'm afraid we can't accept it like that. My humble opinion again :) (In reply to comment #5) > We have buildbots reasonably testing the current player. If possible it'd be better to merge both players now already, in my opinion. > > Or minimize the size of that new player, in that state i'm afraid we can't accept it like that. My humble opinion again :) I quite agree with you, but I'm afraid that reduce the player might not be possible. Maybe the right way to go is to merge them now. I will work on it. Also, my concern is about the CentralPipeLine unit. Is a global/static unit that controls/splits the gst elements thought the units who might use it. Might looks bloat to getUserMedia API, but it's a important component for the PeerConnection API. Is it ok to keep static/global units on webkit1? (In reply to comment #6) > (In reply to comment #5) > Also, my concern is about the CentralPipeLine unit. Is a global/static unit that controls/splits the gst elements thought the units who might use it. Might looks bloat to getUserMedia API, but it's a important component for the PeerConnection API. Is it ok to keep static/global units on webkit1? Hum I'm not familiar enough with those APIs to answer that question properly I think... How is it implemented in chromium? Comment on attachment 170410 [details] core patch Attachment 170410 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14546306 Created attachment 185577 [details]
Patch
Comment on attachment 185577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185577&action=review Ok it looks good but I haven't made a full review because the patch is not based on ToT. Can you rebase it please? There were several changes recently including audio pitch preservation and native fullscreen support (with GStreamerGWorld). Having a rebased patch will also allow the EWS to chew this up :) > Source/WebCore/ChangeLog:9 > + [gtk] Refactor the media player implementation so more of the > + internal functionality can be shared between the current media > + backend and the mediastream player backend. Common code is > + broken out into a MediaPlayerPrivateGStreamerBase, and both > + MediaPlayerPrivateGStreamer and > + StreamMediaPlayerPrivateGStreamer inherit from this base class. > + https://bugs.webkit.org/show_bug.cgi?id=100261 The change description is ok but doesn't comply with the ChangeLog format :) Usually it's: bug title bug url Reviewed by .... change description > Source/WebCore/ChangeLog:13 > + No new tests (OOPS!). You can remove this line or simply state existing media tests cover this patch. > Source/WebCore/GNUmakefile.list.am:6065 > + Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp \ > + Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h \ The Qt and EFL build systems need similar changes. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:262 > +#if 0 // implementation from StreamMediaPlayer Perhaps this block can be removed for now > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:28 > +typedef struct _WebKitVideoSink WebKitVideoSink; You can remove this I think > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:35 > +class GStreamerGWorld; Ditto Created attachment 186444 [details]
Patch
Attachment 186444 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/platform/graphics/MediaPlayer.cpp', u'Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h', u'Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.cpp', u'Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.h', u'configure.ac']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.h:38: The parameter name "player" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 186444 [details] Patch Attachment 186444 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16375453 Comment on attachment 186444 [details] Patch Attachment 186444 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16375482 Comment on attachment 186444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186444&action=review Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:156 > + guint m_videoCapsHandler; This doesn't seem used anywhere? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:283 > + // FIXME: use implementation from StreamMediaPlayer?? I don't understand this FIXME, is it a left-over from a previous version of the patch? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:368 > +// creates and initializes a bunch of internal internal variables, and returns a Double "internal" :) Also comments should be full sentences. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110 > + GRefPtr<GstStreamVolume> m_volume; This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch. > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:126 > + // go go go Hum :) > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:89 > + GstElement* m_videoSinkBin; > + GstElement* m_audioSinkBin; Why not reuse the ones from the base class? (In reply to comment #15) > Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch? Yeah, I was debating that as well. I can do that if you'd like. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:156 > > + guint m_videoCapsHandler; > > This doesn't seem used anywhere? Oops, left over from a different approach > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:283 > > + // FIXME: use implementation from StreamMediaPlayer?? > > I don't understand this FIXME, is it a left-over from a previous version of the patch? yeah, I'll remove. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110 > > + GRefPtr<GstStreamVolume> m_volume; > > This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch. Well, since there's no template specialization for GstStreamVolume, it should just use the un-specialized implementation (GRefPtr<T>), which is the implementation for GObject (g_object_ref_sink/g_object_unref). Since GstStreamVolume has 'GObject' as its only prerequisite, this is probably correct behavior, right? > > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:126 > > + // go go go > > Hum :) Hmm, left over from the implementation. if I remove the stream player from this patch, we can deal with that later :) > > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:89 > > + GstElement* m_videoSinkBin; > > + GstElement* m_audioSinkBin; > > Why not reuse the ones from the base class? Oops, thanks for catching that. (In reply to comment #16) > (In reply to comment #15) > > Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch? > > Yeah, I was debating that as well. I can do that if you'd like. > Yes please! Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110 > > > + GRefPtr<GstStreamVolume> m_volume; > > > > This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch. > > Well, since there's no template specialization for GstStreamVolume, it should just use the un-specialized implementation (GRefPtr<T>), which is the implementation for GObject (g_object_ref_sink/g_object_unref). Since GstStreamVolume has 'GObject' as its only prerequisite, this is probably correct behavior, right? > Ah right indeed. Thanks for clarifying :) Created attachment 186691 [details]
Patch
Comment on attachment 186691 [details]
Patch
Ok it looks good but then the bug title doesn't make sense anymore since the patch is now a pure refactoring, can you retitle and update ChangeLogs accordingly before landing this?
Created attachment 186864 [details]
Patch
Comment on attachment 186864 [details]
Patch
No need for r+ I think since you've set the Reviewed by field already.
Comment on attachment 186864 [details] Patch Clearing flags on attachment: 186864 Committed r142005: <http://trac.webkit.org/changeset/142005> All reviewed patches have been landed. Closing bug. |