Skip to content

[GStreamer] Video not resized with playbin3 on i.MX8M Plus and COG#16901

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
marex:gst-pb3
Aug 31, 2023
Merged

[GStreamer] Video not resized with playbin3 on i.MX8M Plus and COG#16901
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
marex:gst-pb3

Conversation

@marex
Copy link

@marex marex commented Aug 21, 2023

7d6cd5e

[GStreamer] Video not resized with playbin3 on i.MX8M Plus and COG
https://bugs.webkit.org/show_bug.cgi?id=260482

Reviewed by Philippe Normand.

When using WEBKIT_GST_USE_PLAYBIN3=1 and cog to open h264 video (e.g.
Big Buck Bunny h264 1920x1080 mp4), then only single message is delivered
to MediaPlayerPrivateGStreamer::handleStreamCollectionMessage() . The
current workaround check would bail out since the message source is
decodebin3, while the m_source is filesrc . This prevents player->updateTracks
from being called, and thus m_hasVideo from being set, and thus
hasVideo() in MediaPlayerPrivateGStreamer::naturalSize() returns false
and MediaPlayerPrivateGStreamer::naturalSize returns FloatSize()
size, which is 0x0. The resulting video element in the browser has
minimum height set and is not correctly resized.

Limit the MediaPlayerPrivateGStreamer::handleStreamCollectionMessage()
workaround only to non-filesrc, since filesrc is not generating the
stream-collection events.

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::handleStreamCollectionMessage):

Canonical link: https://commits.webkit.org/267515@main

79efdef

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@marex marex requested a review from philn as a code owner August 21, 2023 21:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 22, 2023
@philn
Copy link
Member

philn commented Aug 22, 2023

Seems like this is breaking 2 tests, Found 2 new test failures: imported/w3c/web-platform-tests/media-source/mediasource-changetype-play-without-codecs-parameter.html, imported/w3c/web-platform-tests/media-source/mediasource-detach.html

I'm on sick leave, assigning calvaris to review

@philn philn requested review from calvaris and removed request for philn August 22, 2023 08:03
@calvaris
Copy link
Contributor

Well, the patch looks good, but if tests fail, they need to be investigated by @marex before proceeding.

@calvaris
Copy link
Contributor

It looks like tests keep failing.

@marex
Copy link
Author

marex commented Aug 31, 2023

@calvaris Can you provide any hints what to do about that test ? I don't even have any apple hardware to test on.

@philn
Copy link
Member

philn commented Aug 31, 2023

It looks like tests keep failing.

I see no failure? The mac one is false-positive.

@philn
Copy link
Member

philn commented Aug 31, 2023

Can you remove this from the commit message?

Limit the MediaPlayerPrivateGStreamer::handleStreamCollectionMessage()
workaround only to gstreamer releases before 1.20.y, since this bug
seems to be fixed in 1.20.y and newer.

@marex
Copy link
Author

marex commented Aug 31, 2023

Can you remove this from the commit message?

Limit the MediaPlayerPrivateGStreamer::handleStreamCollectionMessage() workaround only to gstreamer releases before 1.20.y, since this bug seems to be fixed in 1.20.y and newer.

The description of the MR should now be synchronized with the patch.

@philn philn added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue labels Aug 31, 2023
https://bugs.webkit.org/show_bug.cgi?id=260482

Reviewed by Philippe Normand.

When using WEBKIT_GST_USE_PLAYBIN3=1 and cog to open h264 video (e.g.
Big Buck Bunny h264 1920x1080 mp4), then only single message is delivered
to MediaPlayerPrivateGStreamer::handleStreamCollectionMessage() . The
current workaround check would bail out since the message source is
decodebin3, while the m_source is filesrc . This prevents player->updateTracks
from being called, and thus m_hasVideo from being set, and thus
hasVideo() in MediaPlayerPrivateGStreamer::naturalSize() returns false
and MediaPlayerPrivateGStreamer::naturalSize returns FloatSize()
size, which is 0x0. The resulting video element in the browser has
minimum height set and is not correctly resized.

Limit the MediaPlayerPrivateGStreamer::handleStreamCollectionMessage()
workaround only to non-filesrc, since filesrc is not generating the
stream-collection events.

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::handleStreamCollectionMessage):

Canonical link: https://commits.webkit.org/267515@main
@webkit-commit-queue
Copy link
Collaborator

Committed 267515@main (7d6cd5e): https://commits.webkit.org/267515@main

Reviewed commits have been landed. Closing PR #16901 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 31, 2023
@webkit-commit-queue webkit-commit-queue merged commit 7d6cd5e into WebKit:main Aug 31, 2023
@calvaris
Copy link
Contributor

calvaris commented Sep 1, 2023

Ah, sorry, I misread the bot name and thought it was one of the linux ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants