Bug 174582 – [GTK] HTMLMediaElement resize event not fired when video size changes

• Component: Media
• Status: RESOLVED FIXED

[Nael Ouedraogo]

[GTK] HTMLMediaElement resize event not fired when video size change.

[Nael Ouedraogo]

Created attachment 315664 [details]
Patch

[Nael Ouedraogo]

Created attachment 315667 [details]
Patch

[Build Bot]

Comment on attachment 315667 [details]
Patch

Attachment 315667 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4136505

New failing tests:
storage/websql/execute-sql-rowsAffected.html

[Build Bot]

Created attachment 315684 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Nael Ouedraogo]

Failing tests on ios-sim seems not related to this patch.

[Enrique Ocaña]

This fix has a small problem: AppendPipeline::m_oldTrack and m_track aren't being currently updated on caps changes (and probably they shouldn't, that smells to legacy code). Anyway, as they're not updated, the code in MediaPlayerPrivateGStreamerMSE::trackDetected() will always call PlaybackPipeline::attachTrack() and never call reattachTrack() (the proper thing to do on caps changes). AttachTrack() creates a new stream bin in WebKitMediaSrc, and that's not good.

If reattachTrack() was called, the old stream bin would be reused, which is the right thing to happen.

Please, could you make it work in that way? Maybe you will need to change the signature of trackDetected() to signal the purpose of reattaching with a boolean instead of with the old track parameter (currently useless?).

Thanks.

[Nael Ouedraogo]

Created attachment 321527 [details]
Patch

[Nael Ouedraogo]

Thanks for the review. In uploaded patch, I removed oldTrack and call reattachTrack as suggested. I also modified  reattachTrack since caps of stream's appsrc are not valid when calling reattachTrack.

[Enrique Ocaña]

Comment on attachment 321527 [details]
Patch

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

Apart from the minor comment below, the rest is ok. I've checked locally that it causes no test regressions and, indeed, the new passing tests actually pass.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:83
> +    void trackDetected(RefPtr<AppendPipeline>, RefPtr<WebCore::TrackPrivateBase> newTrack, bool);

Leave the newTrack parameter name empty. It's not needed now that there are no confusion betweet two parameters with the same name. Add a parameter name for the bool parameter. See: https://webkit.org/code-style-guidelines/#names-variable-name-in-function-decl

[Xabier Rodríguez Calvar]

Comment on attachment 321527 [details]
Patch

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

On top of Enrique's, I add two small change requests.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:303
> +void PlaybackPipeline::reattachTrack(RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivate, RefPtr<TrackPrivateBase> trackPrivate, const gchar* mediaType)

gchar -> char

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.h:61
> +    void reattachTrack(RefPtr<SourceBufferPrivateGStreamer>, RefPtr<TrackPrivateBase>, const gchar*);

gchar -> char

[Nael Ouedraogo]

Created attachment 321541 [details]
Patch

[Nael Ouedraogo]

Thanks for the reviews.

[WebKit Commit Bot]

Comment on attachment 321541 [details]
Patch

Clearing flags on attachment: 321541

Committed r222388: <http://trac.webkit.org/changeset/222388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694280>