Bug 204352 – [GStreamer] Flatten MediaPlayerPrivateGStreamer into MediaPlayerPrivateGStreamerBase

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

[GStreamer] Flatten MediaPlayerPrivateGStreamer into MediaPlayerPrivateGStreamerBase

[Charlie Turner]

Created attachment 383867 [details]
Patch

[Charlie Turner]

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.

[Charlie Turner]

Created attachment 383868 [details]
Patch

[Charlie Turner]

Created attachment 383871 [details]
Patch

[Charlie Turner]

The style errors are false positives.

[Xabier Rodríguez Calvar]

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

[Philippe Normand]

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 ( ?

[Xabier Rodríguez Calvar]

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?

[Charlie Turner]

Created attachment 384280 [details]
Patch

[Charlie Turner]

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.

[Charlie Turner]

Created attachment 384284 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384284 [details]
Patch for landing

Clearing flags on attachment: 384284

Committed r252852: <https://trac.webkit.org/changeset/252852>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xabier Rodríguez Calvar]

I miss the follow up bug for my comments