Bug 219493 - [GStreamer] Fix video losing size at the end of the stream
Summary: [GStreamer] Fix video losing size at the end of the stream
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-03 09:24 PST by Alicia Boya García
Modified: 2020-12-03 13:13 PST (History)
15 users (show)

See Also:


Attachments
Patch (52.77 KB, patch)
2020-12-03 09:35 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch for landing (52.12 KB, patch)
2020-12-03 12:44 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].