Bug 237359

Summary: Scope capture sources by page identifiers
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, jonlee, philipj, ryuan.choi, 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=211509
Bug Depends on:    
Bug Blocks: 237316    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing - v2
none
Patch for landing - v3
none
Rebasing
none
Rebasing
none
Patch for landing none

Description youenn fablet 2022-03-02 03:48:24 PST
Scope capture sources by page identifiers
Comment 1 youenn fablet 2022-03-02 03:51:35 PST
Created attachment 453585 [details]
Patch
Comment 2 youenn fablet 2022-03-02 04:05:29 PST
Created attachment 453586 [details]
Patch
Comment 3 youenn fablet 2022-03-02 04:16:45 PST
Created attachment 453587 [details]
Patch
Comment 4 youenn fablet 2022-03-02 05:25:46 PST
Created attachment 453591 [details]
Patch
Comment 5 youenn fablet 2022-03-02 05:34:40 PST
Created attachment 453592 [details]
Patch
Comment 6 youenn fablet 2022-03-02 06:09:01 PST
Created attachment 453596 [details]
Patch
Comment 7 youenn fablet 2022-03-02 08:15:35 PST
Comment on attachment 453596 [details]
Patch

GTK failure is legit and due to difference of -expected.txt files, I'll fix that. Putting as r? while waiting for iOS results.
Comment 8 Eric Carlson 2022-03-02 08:45:04 PST
Comment on attachment 453596 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:328
> +            if (mediaConstraints) {
> +                auto error = source->applyConstraints(*mediaConstraints);
> +                if (error)
> +                    return WTFMove(error->message);
> +            }

It would be nice to improve this eventually so we apply a union of the constraints, so we always capture at the highest requested constraint values and decimate/downsample as necessary. Maybe add a FIXME?

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:343
> +#if PLATFORM(IOS_FAMILY)
> +    if (perPageSources.cameraSource) {
> +        // We cannot reuse the source so fail it.
> +        perPageSources.cameraSource->captureFailed();
> +        perPageSources.cameraSource->stop();
> +    }
> +#endif

I would be cleaner to have the compile flag in a method (`canCaptureFromMultipleCameras`?).
Comment 9 youenn fablet 2022-03-04 03:05:54 PST
Created attachment 453828 [details]
Patch for landing
Comment 10 youenn fablet 2022-03-04 03:20:49 PST
> > Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:328
> > +            if (mediaConstraints) {
> > +                auto error = source->applyConstraints(*mediaConstraints);
> > +                if (error)
> > +                    return WTFMove(error->message);
> > +            }
> 
> It would be nice to improve this eventually so we apply a union of the
> constraints, so we always capture at the highest requested constraint values
> and decimate/downsample as necessary. Maybe add a FIXME?

RealtimeVideoCaptureSource does this for width, height and frameRate.
This should ensure there is no functionality regression.

> > Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:343
> > +#if PLATFORM(IOS_FAMILY)
> > +    if (perPageSources.cameraSource) {
> > +        // We cannot reuse the source so fail it.
> > +        perPageSources.cameraSource->captureFailed();
> > +        perPageSources.cameraSource->stop();
> > +    }
> > +#endif
> 
> I would be cleaner to have the compile flag in a method
> (`canCaptureFromMultipleCameras`?).

Done
Comment 11 youenn fablet 2022-03-04 03:47:43 PST
Created attachment 453829 [details]
Patch for landing
Comment 12 youenn fablet 2022-03-04 08:35:11 PST
Comment on attachment 453829 [details]
Patch for landing

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:295
> +    if (failSilently == FailSilently::No)

Instead of introducing this boolean, I'll add a dedicated method instead.
Comment 13 youenn fablet 2022-03-04 08:37:21 PST
Created attachment 453846 [details]
Patch for landing - v2
Comment 14 youenn fablet 2022-03-07 00:29:53 PST
Created attachment 453946 [details]
Patch for landing - v3
Comment 15 youenn fablet 2022-03-08 04:32:40 PST
Created attachment 454101 [details]
Rebasing
Comment 16 youenn fablet 2022-03-08 04:34:37 PST
Created attachment 454102 [details]
Rebasing
Comment 17 youenn fablet 2022-03-08 05:36:51 PST
Created attachment 454111 [details]
Patch for landing
Comment 18 EWS 2022-03-09 00:05:55 PST
Committed r291034 (248208@main): <https://commits.webkit.org/248208@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454111 [details].
Comment 19 Radar WebKit Bug Importer 2022-03-09 00:06:30 PST
<rdar://problem/90016088>
Comment 20 Jon Lee 2022-03-10 13:11:02 PST
This fixed platform/ios/mediastream/audio-muted-in-background-tab-gpu-process.html, originally marked as failing in bug 211509. Removed test expectation in r291125.