[GStreamer] Flatten MediaPlayerPrivateGStreamer into MediaPlayerPrivateGStreamerBase
Created attachment 383867 [details] Patch
This patch is perhaps much too large to review, and due to the merging of one file into another, the diffs are impossible to follow. I haven't actually changed any code, apart from to move some ctor-initialized variables to in-class initialized variables to avoid fiddly work satisfying -Wreorder.
Created attachment 383868 [details] Patch
Created attachment 383871 [details] Patch
The style errors are false positives.
Comment on attachment 383871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383871&action=review I know that many of the comments I am doing here come from the original code, but considering that we are going to pollute the diff, I think we should fix the detected style issues. There are more for sure. I stick to the r+ because required changes should be trivial. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:247 > + } else > +#else > + UNUSED_PARAM(flags); > + UNUSED_PARAM(gstGLEnabled); > +#endif // USE(GSTREAMER_GL) > + > + { > + m_textureID = 0; > + m_isMapped = gst_video_frame_map(&m_videoFrame, &videoInfo, m_buffer, GST_MAP_READ); > + if (m_isMapped) { > + // Right now the TextureMapper only supports chromas with one plane > + ASSERT(GST_VIDEO_INFO_N_PLANES(&videoInfo) == 1); > + } > + } Does this build without GStreamer GL support? The comment needs a period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:273 > + GLuint textureID() const { return m_textureID; } If this does not come from the superclass, I think we should move to textureId (all the way). > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:394 > +) > +#if USE(TEXTURE_MAPPER_GL) > +#if USE(NICOSIA) > + , m_nicosiaLayer(Nicosia::ContentLayer::create(Nicosia::ContentLayerTextureMapperImpl::createFactory(*this))) > +#else > + , m_platformLayerProxy(adoptRef(new TextureMapperPlatformLayerProxy())) > +#endif > +#endif Shouldn't this be included before the ( ? I think we need to proof build with several options. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:677 > + // If we are at beginning of media, start from the end to > + // avoid immediate EOS. This can be one line. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:681 > + if (position < MediaTime::zeroTime()) > + endTime = durationMediaTime(); > + else > + endTime = position; This can be a ? : assignment > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:838 > + // Higher rate causes crash. > + rate = clampTo(rate, -20.0, 20.0); Clamping is fine but I would warn as well (either g_warn or WebKit WARN). > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:842 > + // and make sure that upper layers were notified if rate was set Capital and period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:850 > + // notify upper layers that we cannot handle passed rate. Capital > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:857 > + GstState state; > + GstState pending; No strong opinion, but my preference for this would be "GstState state, pending;". > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:930 > + // Fallback to the more general maxTimeLoaded() if no range has > + // been found. Single line. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:951 > + // infinite duration means live stream Capital and period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1008 > + gint64 length = 0; uint64_t > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1017 > + // Fall back to querying the source pads manually. > + // See also https://bugzilla.gnome.org/show_bug.cgi?id=638749 Single line and period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1025 > + gint64 padLength = 0; uint64_t > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1173 > + GstState currentState; > + GstState pending; Ditto. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1198 > + // Create a timer when entering the READY state so that we can free resources > + // if we stay for too long on READY. > + // Also lets remove the timer if we request a state change for any state other than READY. > + // See also https://bugs.webkit.org/show_bug.cgi?id=117354 Please consider two lines for this and the period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1201 > + // Max interval in seconds to stay in the READY state on manual > + // state change requests. Single line > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1252 > + gint numTracks = 0; int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1275 > + for (gint i = 0; i < numTracks; ++i) { int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1330 > + gint numTracks = 0; int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1349 > + for (gint i = 0; i < numTracks; ++i) { int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1385 > +void MediaPlayerPrivateGStreamer::notifyPlayerOfText() Are these three functions almost the same? Wouldn't it make sense to use some templating magic here if possible? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1392 > + gint numTracks = 0; int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1405 > + for (gint i = 0; i < numTracks; ++i) { int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1657 > + case TrackPrivateBaseGStreamer::TrackType::Unknown: > + default: Doesn't this need a FALLTHROUGH? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1670 > + // TODO: MSE GstStream API support: https://bugs.webkit.org/show_bug.cgi?id=182531 . > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1708 > + if ((oldHasVideo != m_hasVideo) || (oldHasAudio != m_hasAudio)) No need for inner () here. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1767 > + const gchar* contextType; char > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1827 > + GST_WARNING("CDM instance not initializaed"); typo! > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1878 > + if (shouldAdoptRef) > + m_glDisplay = adoptGRef(GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(downcast<PlatformDisplayX11>(sharedDisplay).native()))); > + else > + m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(downcast<PlatformDisplayX11>(sharedDisplay).native())); I think we can do either a macro or a static inline function for this, including the webkit gst version check that could be made static. Please apply to all appearances of the pattern. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1952 > +// Returns the size of the video period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1968 > + gint width, height; int > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1993 > + // TODO: handle possible clean aperture data. See > + // https://bugzilla.gnome.org/show_bug.cgi?id=596571 > + // TODO: handle possible transformation matrix. See > + // https://bugzilla.gnome.org/show_bug.cgi?id=596326 Two lines, periods. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2024 > + guint64 width = 0, height = 0; uint64_t > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2186 > + // Let the mediaPlayerClient handle the stream error, in > + // this case the HTMLMediaElement will emit a stalled Single line, period. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2513 > + for (guint j = 0; j < stream->descriptors->len; ++j) { unsigned > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2515 > + for (guint k = 0; k < descriptor->length; ++k) unsigned > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2563 > + gint64 start = -1, stop = -1; uint64_t > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2572 > + gchar* title = nullptr; char > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2703 > + GstState pending; > + GstState state; Collapse. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2808 > + // Change failed . > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2889 > + const gchar* newLocation = nullptr; char > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3054 > + const gchar* playbinName = "playbin"; char > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3078 > + // gst_element_factory_make() returns a floating reference so > + // we should not adopt. Is this comment valid? I don't see any adoption or smart ptr anywhere. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3288 > + bool triggerResize; shouldTriggerResize. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3449 > + const gchar* formatString = GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? "RGBA" : "BGRx"; > +#else > + const gchar* formatString = GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? "RGBA" : "RGBx"; char > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3636 > + gboolean result = TRUE; bool result = true; > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3829 > + guint64 decodedFrames = 0; uint64_t > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3837 > + guint64 framesDropped = 0; ditto > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3846 > + gint64 position = 0; int64_t > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3858 > + gint64 position = 0; ditto > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3935 > + bool eventHandled = gst_element_send_event(pipeline(), gst_event_new_custom(GST_EVENT_CUSTOM_DOWNSTREAM_OOB, gst_structure_new_empty("attempt-to-decrypt"))); wasEventHandled > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3955 > +void MediaPlayerPrivateGStreamer::setWaitingForKey(bool waitingForKey) parameter isWaitingForKey > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:5 > - * Copyright (C) 2009, 2010, 2011, 2012, 2013, 2015, 2016 Igalia S.L > - * Copyright (C) 2014 Cable Television Laboratories, Inc. > + * Copyright (C) 2009, 2010, 2015, 2016 Igalia S.L Why u removing CableLabs? We need to update the year here as well. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:213 > + unsigned decodedFrameCount() const override; As an example, could you please check all overrides here against the MSE player and make final here the ones that do not appear there? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:327 > + // FIXME: Where is this used? > + void handlePluginInstallerResult(GstInstallPluginsReturn); If it is used nowhere, please remove it. What is weird is that the compiler shuts up about this... > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:369 > + bool m_changingRate { false }; > + bool m_downloadFinished { false }; > + bool m_errorOccured { false }; Let's take advantage to fix the style here: m_isChangingRate, m_didDownloadFinish, m_didErrorOccur. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:372 > + bool m_paused { true }; m_isPaused > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:379 > + bool m_resetPipeline { false }; > + bool m_seeking { false }; > + bool m_seekIsPending { false }; m_shouldResetPipeline m_isSeeking m_isSeekPending; > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:382 > + bool m_volumeAndMuteInitialized { false }; m_areVolumeAndMuteInitialized > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:403 > + bool m_renderingCanBeAccelerated { false }; > + > + bool m_destroying { false }; m_isUsingFallbackVideoSink, m_canRenderingBeAccelerated, m_isBeingDestroyed > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:426 > + bool m_waitingForKey { false }; m_isWaitingForKey > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:495 > + bool m_playbackRatePause { false }; m_isPlaybackRatePaused > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:502 > + bool m_delayingLoad { false }; m_isDelayingLoad > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:517 > + bool m_buffering { false }; m_isBuffering > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:521 > + bool m_preservesPitch { false }; m_shouldPreservePitch
Comment on attachment 383871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383871&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:247 >> + } > > Does this build without GStreamer GL support? > > The comment needs a period. I don't see why it would not. What code exactly makes you think there's a missing ifdef? >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:394 >> +#endif > > Shouldn't this be included before the ( ? I think we need to proof build with several options. Which ( ?
Comment on attachment 383871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383871&action=review >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:247 >>> + } >> >> Does this build without GStreamer GL support? >> >> The comment needs a period. > > I don't see why it would not. What code exactly makes you think there's a missing ifdef? Oh, yes, my bad. I misread the code. >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:394 >>> +#endif >> >> Shouldn't this be included before the ( ? I think we need to proof build with several options. > > Which ( ? I mean ) and I read wrong again, though it is true that m_readyTimerHander is misplaced and should be moved just to the line above. Right?
Created attachment 384280 [details] Patch
I disagree with performing these cleanups in this patch. I have fixed the copyright issue I introduced in the header, but apart from that I will land this with the minimal set of cleanups possible. They can be performed in follow-up patches once the dust had settled.
Created attachment 384284 [details] Patch for landing
Comment on attachment 384284 [details] Patch for landing Clearing flags on attachment: 384284 Committed r252852: <https://trac.webkit.org/changeset/252852>
All reviewed patches have been landed. Closing bug.
I miss the follow up bug for my comments