Bug 219493

Summary: [GStreamer] Fix video losing size at the end of the stream
Product: WebKit Reporter: Alicia Boya García <aboya>
Component: WebKitGTKAssignee: Alicia Boya García <aboya>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, clopez, eric.carlson, ews-watchlist, glenn, gns, jer.noble, menard, philipj, pnormand, sergio, vjaquez, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/26743
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Alicia Boya García 2020-12-03 09:24:42 PST
sue where at the end of the video the
tracks would be erased, causing the video to lose its size and by
extension its aspect ratio.

In absence of a size, WebKit uses the default video size defined by
the spec of 300x150 (2:1 aspect ratio). This causes a video element
that doesn't have a size set through CSS to shrink to that size at the
end of playback, and also for black bars to appear on wider content
(e.g. 16:9 video) when watched in full screen mode.

This patch fixes the problem by not removing the tracks after an end
of stream, and instead reusing them with different pads the next time
the video is played.
Comment 1 Alicia Boya García 2020-12-03 09:25:35 PST
(In reply to Alicia Boya García from comment #0)
> sue where at the end of the video the
> tracks would be erased, causing the video to lose its size and by
> extension its aspect ratio.

The first paragraph got cut:

        Our port for long had an issue where at the end of the video the
        tracks would be erased, causing the video to lose its size and by
        extension its aspect ratio.
Comment 2 Alicia Boya García 2020-12-03 09:35:04 PST
Created attachment 415305 [details]
Patch
Comment 3 EWS Watchlist 2020-12-03 09:35:44 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 Alicia Boya García 2020-12-03 09:40:26 PST
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/26743
Comment 5 Xabier Rodríguez Calvar 2020-12-03 11:00:51 PST
Comment on attachment 415305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415305&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1066
> +                    existingTrack->setPad(GRefPtr(pad));

Nit: Unless that for some reason you want to keep it explicit, you can remove the GRefPtr constructor that will be called implicitly.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1147
> +                    existingTrack->setPad(GRefPtr(pad));

Ditto.

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:179
> +void TrackPrivateBaseGStreamer::setPad(GRefPtr<GstPad>&& pad)
> +{
> +    m_pad = WTFMove(pad);
> +}

Nit: I think you can inline this in the header file.
Comment 6 Alicia Boya García 2020-12-03 11:31:52 PST
Comment on attachment 415305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415305&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1066
>> +                    existingTrack->setPad(GRefPtr(pad));
> 
> Nit: Unless that for some reason you want to keep it explicit, you can remove the GRefPtr constructor that will be called implicitly.

Unfortunately that's not the case. setPad() expects an rvalue reference, but since here we don't want to do a move, we have to create a new one by coping the reference. Otherwise you get this error:

error: cannot bind rvalue reference of type ‘WTF::GRefPtr<_GstPad>&&’ to lvalue of type ‘WTF::GRefPtr<_GstPad>’
Comment 7 Xabier Rodríguez Calvar 2020-12-03 11:58:47 PST
Comment on attachment 415305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415305&action=review

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1066
>>> +                    existingTrack->setPad(GRefPtr(pad));
>> 
>> Nit: Unless that for some reason you want to keep it explicit, you can remove the GRefPtr constructor that will be called implicitly.
> 
> Unfortunately that's not the case. setPad() expects an rvalue reference, but since here we don't want to do a move, we have to create a new one by coping the reference. Otherwise you get this error:
> 
> error: cannot bind rvalue reference of type ‘WTF::GRefPtr<_GstPad>&&’ to lvalue of type ‘WTF::GRefPtr<_GstPad>’

Ah, sorry, I thought pad was a GstPad* instead of a GRefPtr<GstPad>. Forget this.
Comment 8 Alicia Boya García 2020-12-03 12:44:36 PST
Created attachment 415325 [details]
Patch for landing
Comment 9 EWS 2020-12-03 13:13:00 PST
Committed r270404: <https://trac.webkit.org/changeset/270404>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415325 [details].