Bug 228941

Summary: REGRESSION(r280732) [GStreamer] fast/mediastream/getDisplayMedia-max-constraints1.html and other are failing
Product: WebKit Reporter: Lauro Moura <lmoura>
Component: MediaAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, pnormand, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=228759
https://bugs.webkit.org/show_bug.cgi?id=210926
https://bugs.webkit.org/show_bug.cgi?id=229343
https://bugs.webkit.org/show_bug.cgi?id=229868
Attachments:
Description Flags
Patch
none
Patch youennf: review+

Lauro Moura
Reported 2021-08-09 19:46:21 PDT
fast/mediastream/getDisplayMedia-max-constraints1.html fast/mediastream/getDisplayMedia-max-constraints2.html After r280732 (240319@main) (partial revert from r279940) Both tests fail/time out while waiting for the expectedWidth, which always comes unadjusted for the aspect ratio. e.g. the first test expects 281 for height but only 500 is arriving. While inspecting the mockvideosource code, indeed the source has its size changed to the expected value, but this doesn't seems to be reflected in the track settings attribute.
Attachments
Patch (3.44 KB, patch)
2021-08-16 04:22 PDT, Philippe Normand 		no flags
DetailsFormatted DiffDiff
Patch (13.28 KB, patch)
2021-08-19 05:00 PDT, Philippe Normand 		youennf: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Philippe Normand
Comment 1 2021-08-16 04:22:58 PDT
Created attachment 435586 [details] Patch
youenn fablet
Comment 2 2021-08-16 05:49:46 PDT
Comment on attachment 435586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435586&action=review > Source/WebCore/ChangeLog:9 > + GStreamer ports differ in that regard, in their mock source implementations. Diverting here is probably not very healthy, though it is true that cocoa port is doing capture out of process and GStreamer is probably not. Is it only the mock sources that differ or is it also the case for actual sources? If the issue is with mock, can we realign mocks? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:119 > if (size.isEmpty()) Do you know why size is not empty here for those tests in GStreamer but is empty for Cocoa port?
Philippe Normand
Comment 3 2021-08-16 06:29:03 PDT
In GStreamer ports we use our mock video source for mock'ing display capture, while the mac ports use a custom mock display capture source, MockDisplayCapturer (inherits from DisplayCaptureSourceCocoa::Capturer. For us the size is not empty because it's set from the settings, whereas your Capturer seems to delay that to until the first frame has been generated. I haven't verified this though.
youenn fablet
Comment 4 2021-08-16 06:40:15 PDT
> For us the size is not empty because it's set from the settings Where is it set in the source code? Does it mirror what the GStreamer capture source is doing?
Philippe Normand
Comment 5 2021-08-17 07:17:30 PDT
This code in the MockRealtimeVideoSource ctor: if (mockDisplay()) { auto& properties = WTF::get<MockDisplayProperties>(m_device.properties); setIntrinsicSize(properties.defaultSize); setSize(properties.defaultSize); m_fillColor = properties.fillColor; return; }
Philippe Normand
Comment 6 2021-08-17 07:35:17 PDT
Disabling that setSize call gets me passing tests :P
Philippe Normand
Comment 7 2021-08-17 07:51:03 PDT
(In reply to Philippe Normand from comment #6) > Disabling that setSize call gets me passing tests :P Hum no, this needs more debugging...
Philippe Normand
Comment 8 2021-08-19 05:00:59 PDT
Created attachment 435857 [details] Patch
youenn fablet
Comment 9 2021-08-19 06:31:56 PDT
Comment on attachment 435857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435857&action=review > Source/WebCore/platform/mediastream/gstreamer/MockRealtimeVideoSourceGStreamer.h:31 > class MockRealtimeVideoSourceGStreamer final : public MockRealtimeVideoSource { It seems MockRealtimeVideoSourceGStreamer is only used for display. Should its name be updated? Should it be merged with MockDisplayCaptureSourceGStreamer at some point? > Source/WebCore/platform/mediastream/gstreamer/MockRealtimeVideoSourceGStreamer.h:47 > + MockDisplayCaptureSourceGStreamer(RefPtr<MockRealtimeVideoSourceGStreamer>&& source, CaptureDevice::DeviceType type) Can it be made private? Can we pass a Ref<>&& source. > Source/WebCore/platform/mediastream/gstreamer/MockRealtimeVideoSourceGStreamer.h:67 > + RefPtr<MockRealtimeVideoSourceGStreamer> m_source; Can we make it a Ref<>
Philippe Normand
Comment 10 2021-08-19 07:07:05 PDT
Comment on attachment 435857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435857&action=review >> Source/WebCore/platform/mediastream/gstreamer/MockRealtimeVideoSourceGStreamer.h:31 >> class MockRealtimeVideoSourceGStreamer final : public MockRealtimeVideoSource { > > It seems MockRealtimeVideoSourceGStreamer is only used for display. Should its name be updated? Should it be merged with MockDisplayCaptureSourceGStreamer at some point? It's used for mocking cameras gUM as well.
Philippe Normand
Comment 11 2021-08-20 03:17:12 PDT
Committed r281305 (240725@main): <https://commits.webkit.org/240725@main>
Radar WebKit Bug Importer
Comment 12 2021-08-20 03:18:20 PDT
<rdar://problem/82163904>
Note You need to log in before you can comment on or make changes to this bug.