[GStreamer] Make GstMappedFrame assert when it is not initialized #25007

calvaris · 2024-02-23T10:11:12Z

`8ce9ffa`

[GStreamer] Make GstMappedFrame assert when it is not initialized
https://bugs.webkit.org/show_bug.cgi?id=269980

Reviewed by Carlos Garcia Campos.

This way we can align the implementation of GstMappedFrame with GstMappedBuffer and assert when its data is accessed but
it was not properly mapped at initialization.

Even when now it is properly protected at members access, additional checks were added in some places where its
instantiation was not being checked.

A fly-by improvement is making the constructor taking GRefPtr<GstSample> to take const & to avoid unnecessary ref/unref.

* Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:
(WebCore::GstMappedFrame::GstMappedFrame):
(WebCore::GstMappedFrame::get):
(WebCore::GstMappedFrame::ComponentData const):
(WebCore::GstMappedFrame::ComponentStride const):
(WebCore::GstMappedFrame::info):
(WebCore::GstMappedFrame::width const):
(WebCore::GstMappedFrame::height const):
(WebCore::GstMappedFrame::format const):
(WebCore::GstMappedFrame::planeData const):
(WebCore::GstMappedFrame::planeStride const):
(WebCore::GstMappedFrame::isValid const):
(WebCore::GstMappedFrame::operator! const):
* Source/WebCore/platform/graphics/gstreamer/VideoFrameGStreamer.cpp:
(WebCore::VideoFrame::copyTo):
* Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:
(WebCore::GStreamerVideoFrameLibWebRTC::ToI420):

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

2cdc7c7

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	~~🧪 api-wpe~~
🧪 webkitpy	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-02-23T10:11:27Z

EWS run on previous version of this PR (hash ba445ad)

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	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/platform/graphics/gstreamer/ImageGStreamerSkia.cpp

webkit-early-warning-system · 2024-02-23T12:43:36Z

EWS run on previous version of this PR (hash 0f1b9ec)

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	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h

webkit-early-warning-system · 2024-02-23T17:14:47Z

EWS run on previous version of this PR (hash 72fe8e7)

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	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/platform/graphics/gstreamer/VideoFrameGStreamer.cpp

Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp

webkit-early-warning-system · 2024-02-26T14:39:32Z

EWS run on previous version of this PR (hash d95fac8)

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	~~🧪 api-wpe~~
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	~~🧪 api-gtk~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/platform/graphics/gstreamer/VideoFrameGStreamer.cpp

webkit-early-warning-system · 2024-02-26T15:04:20Z

EWS run on previous version of this PR (hash 3ed911a)

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~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 gtk~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-02-26T15:05:50Z

EWS run on current version of this PR (hash 2cdc7c7)

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	~~🧪 api-wpe~~
🧪 webkitpy	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=269980 Reviewed by Carlos Garcia Campos. This way we can align the implementation of GstMappedFrame with GstMappedBuffer and assert when its data is accessed but it was not properly mapped at initialization. Even when now it is properly protected at members access, additional checks were added in some places where its instantiation was not being checked. A fly-by improvement is making the constructor taking GRefPtr<GstSample> to take const & to avoid unnecessary ref/unref. * Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h: (WebCore::GstMappedFrame::GstMappedFrame): (WebCore::GstMappedFrame::get): (WebCore::GstMappedFrame::ComponentData const): (WebCore::GstMappedFrame::ComponentStride const): (WebCore::GstMappedFrame::info): (WebCore::GstMappedFrame::width const): (WebCore::GstMappedFrame::height const): (WebCore::GstMappedFrame::format const): (WebCore::GstMappedFrame::planeData const): (WebCore::GstMappedFrame::planeStride const): (WebCore::GstMappedFrame::isValid const): (WebCore::GstMappedFrame::operator! const): * Source/WebCore/platform/graphics/gstreamer/VideoFrameGStreamer.cpp: (WebCore::VideoFrame::copyTo): * Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp: (WebCore::GStreamerVideoFrameLibWebRTC::ToI420): Canonical link: https://commits.webkit.org/275320@main

webkit-commit-queue · 2024-02-26T15:48:04Z

Committed 275320@main (8ce9ffa): https://commits.webkit.org/275320@main

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

calvaris requested a review from philn as a code owner February 23, 2024 10:11

calvaris self-assigned this Feb 23, 2024

calvaris added the Media Bugs related to the HTML 5 Media elements. label Feb 23, 2024

calvaris requested a review from carlosgcampos February 23, 2024 10:11

carlosgcampos reviewed Feb 23, 2024

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/ImageGStreamerSkia.cpp Outdated Show resolved Hide resolved

calvaris marked this pull request as draft February 23, 2024 11:52

calvaris changed the title ~~[Skia][GStreamer] Small conceptual improvement on ImageGStreamer~~ [GStreamer] Move GstMappedFrame to heap to increase coding safety when instantiation is invalid Feb 23, 2024

calvaris force-pushed the eng/269980 branch from ba445ad to 0f1b9ec Compare February 23, 2024 12:43

calvaris marked this pull request as ready for review February 23, 2024 12:44

calvaris marked this pull request as draft February 23, 2024 13:07

carlosgcampos reviewed Feb 23, 2024

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h Outdated Show resolved Hide resolved

calvaris changed the title ~~[GStreamer] Move GstMappedFrame to heap to increase coding safety when instantiation is invalid~~ [GStreamer] Make GstMappedFrame assert when it is not initialized Feb 23, 2024

calvaris force-pushed the eng/269980 branch from 0f1b9ec to 72fe8e7 Compare February 23, 2024 17:14

calvaris marked this pull request as ready for review February 23, 2024 17:14

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024

carlosgcampos approved these changes Feb 25, 2024

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/VideoFrameGStreamer.cpp Outdated Show resolved Hide resolved

Source/WebCore/platform/mediastream/libwebrtc/gstreamer/GStreamerVideoFrameLibWebRTC.cpp Outdated Show resolved Hide resolved

calvaris removed the merging-blocked Applied to prevent a change from being merged label Feb 26, 2024

calvaris force-pushed the eng/269980 branch from 72fe8e7 to d95fac8 Compare February 26, 2024 14:37

philn requested changes Feb 26, 2024

View reviewed changes

Source/WebCore/platform/graphics/gstreamer/VideoFrameGStreamer.cpp Show resolved Hide resolved

calvaris changed the title ~~[GStreamer] Make GstMappedFrame assert when it is not initialized~~ fixup! [GStreamer] Make GstMappedFrame assert when it is not initialized https://bugs.webkit.org/show_bug.cgi?id=269980 Feb 26, 2024

calvaris changed the title ~~fixup! [GStreamer] Make GstMappedFrame assert when it is not initialized https://bugs.webkit.org/show_bug.cgi?id=269980~~ [GStreamer] Make GstMappedFrame assert when it is not initialized Feb 26, 2024

calvaris force-pushed the eng/269980 branch from 3ed911a to 2cdc7c7 Compare February 26, 2024 15:05

calvaris added the merge-queue Applied to send a pull request to merge-queue label Feb 26, 2024

philn approved these changes Feb 26, 2024

View reviewed changes

webkit-commit-queue force-pushed the eng/269980 branch from 2cdc7c7 to 8ce9ffa Compare February 26, 2024 15:48

webkit-commit-queue merged commit 8ce9ffa into WebKit:main Feb 26, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 26, 2024

calvaris deleted the eng/269980 branch February 26, 2024 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GStreamer] Make GstMappedFrame assert when it is not initialized #25007

[GStreamer] Make GstMappedFrame assert when it is not initialized #25007

calvaris commented Feb 23, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 23, 2024 •

edited

webkit-early-warning-system commented Feb 23, 2024 •

edited

webkit-early-warning-system commented Feb 23, 2024 •

edited

webkit-early-warning-system commented Feb 26, 2024 •

edited

webkit-early-warning-system commented Feb 26, 2024 •

edited

webkit-early-warning-system commented Feb 26, 2024 •

edited

webkit-commit-queue commented Feb 26, 2024

[GStreamer] Make GstMappedFrame assert when it is not initialized #25007

[GStreamer] Make GstMappedFrame assert when it is not initialized #25007

Conversation

calvaris commented Feb 23, 2024 • edited by webkit-early-warning-system

8ce9ffa

webkit-early-warning-system commented Feb 23, 2024 • edited

webkit-early-warning-system commented Feb 23, 2024 • edited

webkit-early-warning-system commented Feb 23, 2024 • edited

webkit-early-warning-system commented Feb 26, 2024 • edited

webkit-early-warning-system commented Feb 26, 2024 • edited

webkit-early-warning-system commented Feb 26, 2024 • edited

webkit-commit-queue commented Feb 26, 2024

calvaris commented Feb 23, 2024 •

edited by webkit-early-warning-system

`8ce9ffa`

webkit-early-warning-system commented Feb 23, 2024 •

edited

webkit-early-warning-system commented Feb 23, 2024 •

edited

webkit-early-warning-system commented Feb 23, 2024 •

edited

webkit-early-warning-system commented Feb 26, 2024 •

edited

webkit-early-warning-system commented Feb 26, 2024 •

edited

webkit-early-warning-system commented Feb 26, 2024 •

edited