+++ This bug was initially created as a clone of Bug #204352 +++ [GStreamer] Flatten MediaPlayerPrivateGStreamer into MediaPlayerPrivateGStreamerBase https://bugs.webkit.org/show_bug.cgi?id=204352#c6
Created attachment 384419 [details] Patch
(In reply to Xabier Rodríguez Calvar from comment #6) > Comment on attachment 383871 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383871&action=review I am skipping all the GL related code since Phil has a patch to refactor all that in-review. A lot of the g* types to C++ types I have also kept. Since we discussed and concluded that the style is to always use g* types for out parameters. Even though in several cases like gint and gchar it's obvious they are the same as int and char, I still use the g* types for out-params for consistency, and favour explicit static_cast'ing to be on the safe side. > > 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. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:681 > > + if (position < MediaTime::zeroTime()) > > + endTime = durationMediaTime(); > > + else > > + endTime = position; > > This can be a ? : assignment Done. > > 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). Done, but I warned with GST_WARNING. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:842 > > + // and make sure that upper layers were notified if rate was set > > Capital and period. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:850 > > + // notify upper layers that we cannot handle passed rate. > > Capital Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:857 > > + GstState state; > > + GstState pending; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:930 > > + // Fallback to the more general maxTimeLoaded() if no range has > > + // been found. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:951 > > + // infinite duration means live stream Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1008 > > + gint64 length = 0; Because this is an out-parameter of gst_element_query_duration, I have kept it as-is, and use an explicit static_cast<uint64_t> to assign to m_totalBytes (which has been changed to be a uint64_t rather than unsigned long long. > > 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 Changed to a single line, but not assing a period since it forms a invalid URL. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1025 > > + gint64 padLength = 0; Out-param. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1173 > > + GstState currentState; > > + GstState pending; Done. > > 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 Done, but see above about invalid URL. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1201 > > + // Max interval in seconds to stay in the READY state on manual > > + // state change requests. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1252 > > + gint numTracks = 0; Done, it's kinda an out-parameter (from g_object_get "n-video"), but it seems fine to use the standard int in this case. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1275 > > + for (gint i = 0; i < numTracks; ++i) { Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1330 > > + gint numTracks = 0; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1349 > > + for (gint i = 0; i < numTracks; ++i) { Done. > > 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? There is a lot of duplication, I'll do that in a follow up since it's easier to break a test with this change. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1392 > > + gint numTracks = 0; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1405 > > + for (gint i = 0; i < numTracks; ++i) { Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1657 > > + case TrackPrivateBaseGStreamer::TrackType::Unknown: > > + default: > > Doesn't this need a FALLTHROUGH? Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1670 > > + // TODO: MSE GstStream API support: https://bugs.webkit.org/show_bug.cgi?id=182531 Invalid URL. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1708 > > + if ((oldHasVideo != m_hasVideo) || (oldHasAudio != m_hasAudio)) > > No need for inner () here. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1767 > > + const gchar* contextType; Kept for consitency with the out-param rule. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1827 > > + GST_WARNING("CDM instance not initializaed"); > > typo! Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1952 > > +// Returns the size of the video > > period. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1968 > > + gint width, height; Kept, out-param. > > 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. Two lines, no periods. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2024 > > + guint64 width = 0, height = 0; > Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2186 > > + // Let the mediaPlayerClient handle the stream error, in > > + // this case the HTMLMediaElement will emit a stalled Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2513 > > + for (guint j = 0; j < stream->descriptors->len; ++j) { Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2515 > > + for (guint k = 0; k < descriptor->length; ++k) Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2563 > > + gint64 start = -1, stop = -1; Out-param. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2572 > > + gchar* title = nullptr; Out-param. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2703 > > + GstState pending; > > + GstState state; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2808 > > + // Change failed > > . Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2889 > > + const gchar* newLocation = nullptr; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3054 > > + const gchar* playbinName = "playbin"; Done. > > 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. It seems to be there to stop future maintainers thinking they should use a smart-pointer here, but they should check the docs instead. Removed. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3288 > > + bool triggerResize; > > shouldTriggerResize. Done. > > 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 Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3636 > > + gboolean result = TRUE; > > bool result = true; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3829 > > + guint64 decodedFrames = 0; > > uint64_t Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3837 > > + guint64 framesDropped = 0; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3846 > > + gint64 position = 0; > > int64_t > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3858 > > + gint64 position = 0; Out-param. > > 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 Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3955 > > +void MediaPlayerPrivateGStreamer::setWaitingForKey(bool waitingForKey) > > parameter isWaitingForKey Done. > > 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? TODO. > > 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... Done. > > 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. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:372 > > + bool m_paused { true }; > > m_isPaused Done. > > 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; Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:382 > > + bool m_volumeAndMuteInitialized { false }; > > m_areVolumeAndMuteInitialized Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:403 > > + bool m_renderingCanBeAccelerated { false }; > > + > > + bool m_destroying { false }; > > m_isUsingFallbackVideoSink, m_canRenderingBeAccelerated, m_isBeingDestroyed Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:426 > > + bool m_waitingForKey { false }; > > m_isWaitingForKey Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:495 > > + bool m_playbackRatePause { false }; > > m_isPlaybackRatePaused Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:502 > > + bool m_delayingLoad { false }; > > m_isDelayingLoad Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:517 > > + bool m_buffering { false }; > > m_isBuffering Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:521 > > + bool m_preservesPitch { false }; > > m_shouldPreservePitch Done.
(In reply to Charlie Turner from comment #2) > I am skipping all the GL related code since Phil has a patch to > refactor all that in-review. > > A lot of the g* types to C++ types I have also kept. Since we > discussed and concluded that the style is to always use g* types for > out parameters. Even though in several cases like gint and gchar it's > obvious they are the same as int and char, I still use the g* types > for out-params for consistency, and favour explicit static_cast'ing to > be on the safe side. Ok. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:857 > > > + GstState state; > > > + GstState pending; > > Done. Did you miss this one? > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1008 > > > + gint64 length = 0; > > Because this is an out-parameter of gst_element_query_duration, I have kept > it as-is, and use an explicit static_cast<uint64_t> to assign to > m_totalBytes (which has been changed to be a uint64_t rather than unsigned > long long. I cannot find this change. > > > 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 > > Changed to a single line, but not assing a period since it forms a invalid > URL. I don't think forming an invalid URL is a problem here, but I am going to buy it. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1173 > > > + GstState currentState; > > > + GstState pending; > > Done. Can't find this one either. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1252 > > > + gint numTracks = 0; > > Done, it's kinda an out-parameter (from g_object_get "n-video"), but it > seems fine to use the standard int in this case. It strikes that you flipped it unsigned even when the API says it is integer, though range seems to be positive range for a signed integer. I don't see this a big issue so I let you decide int or unsigned. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1275 > > > + for (gint i = 0; i < numTracks; ++i) { > > Done. Ditto. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1330 > > > + gint numTracks = 0; > > Done. Ditto. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1349 > > > + for (gint i = 0; i < numTracks; ++i) { > > Done. Ditto. > > > 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? > > There is a lot of duplication, I'll do that in a follow up since it's > easier to break a test with this change. Ok. Please open the bug then. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1392 > > > + gint numTracks = 0; > > Done. Ditto. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1405 > > > + for (gint i = 0; i < numTracks; ++i) { > > Done. Ditto. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1670 > > > + // TODO: MSE GstStream API support: https://bugs.webkit.org/show_bug.cgi?id=182531 > > Invalid URL. Ditto. > > > 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. > > Two lines, no periods. Lol > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2703 > > > + GstState pending; > > > + GstState state; > > Done. This I saw, but not the other two appearances... > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3636 > > > + gboolean result = TRUE; > > > > bool result = true; > > Done. TRUE -> true. > > > 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? > > TODO. Alright, please file bug then.
(In reply to Xabier Rodríguez Calvar from comment #3) > (In reply to Charlie Turner from comment #2) > > > I am skipping all the GL related code since Phil has a patch to > > refactor all that in-review. > > > > A lot of the g* types to C++ types I have also kept. Since we > > discussed and concluded that the style is to always use g* types for > > out parameters. Even though in several cases like gint and gchar it's > > obvious they are the same as int and char, I still use the g* types > > for out-params for consistency, and favour explicit static_cast'ing to > > be on the safe side. > > Ok. > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:857 > > > > + GstState state; > > > > + GstState pending; > > > > Done. > > Did you miss this one? Fixed the remaining cases of this. > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1008 > > > > + gint64 length = 0; > > > > Because this is an out-parameter of gst_element_query_duration, I have kept > > it as-is, and use an explicit static_cast<uint64_t> to assign to > > m_totalBytes (which has been changed to be a uint64_t rather than unsigned > > long long. > > I cannot find this change. That was out-of-date, I initially changed it to uint64_t, but actually higher layers require unsigned long long. > > > > > 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 > > > > Changed to a single line, but not assing a period since it forms a invalid > > URL. > > I don't think forming an invalid URL is a problem here, but I am going to > buy it. My editor includes the period at the end of a URL and takes me to an invalid place. I'll leave them off. > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1173 > > > > + GstState currentState; > > > > + GstState pending; > > > > Done. > > Can't find this one either. Think I got these all now. > > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1252 > > > > + gint numTracks = 0; > > > > Done, it's kinda an out-parameter (from g_object_get "n-video"), but it > > seems fine to use the standard int in this case. > > It strikes that you flipped it unsigned even when the API says it is > integer, though range seems to be positive range for a signed integer. I > don't see this a big issue so I let you decide int or unsigned. I kept it unsigned so that we don't get warnings comparing to the Vector::size() method later on. Since these functions will be getting refactored later, we can leave it as is. 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? > > > > There is a lot of duplication, I'll do that in a follow up since it's > > easier to break a test with this change. > > Ok. Please open the bug then. https://bugs.webkit.org/show_bug.cgi?id=204686 > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3636 > > > > + gboolean result = TRUE; > > > > > > bool result = true; > > > > Done. > > TRUE -> true. This has left us after the GLsink bin. > > > > > 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? > > > > TODO. > > Alright, please file bug then. Actually, I did this in this patch in the end, it was not as much work as I thought.
Created attachment 384461 [details] Patch
Comment on attachment 384461 [details] Patch Really good! Thanks!
Created attachment 384490 [details] Patch for landing
Comment on attachment 384490 [details] Patch for landing Clearing flags on attachment: 384490 Committed r252937: <https://trac.webkit.org/changeset/252937>
All reviewed patches have been landed. Closing bug.
Reverted r252937 for reason: broke GTK/WPE builds and most likely media track notification support Committed r252938: <https://trac.webkit.org/changeset/252938>
Looks like a refactoring was started in this patch regarding track notifications, but was landed unfinished. It wasn't clear to me how to fix the build in a timely manner, hence the rollout.
Created attachment 384516 [details] Patch Remove inadvertent diff from the working dir
Created attachment 384521 [details] Patch for landing
Comment on attachment 384521 [details] Patch for landing Clearing flags on attachment: 384521 Committed r252950: <https://trac.webkit.org/changeset/252950>