Bug 237359 – Scope capture sources by page identifiers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Scope capture sources by page identifiers

[youenn fablet]

Created attachment 453585 [details]
Patch

[youenn fablet]

Created attachment 453586 [details]
Patch

[youenn fablet]

Created attachment 453587 [details]
Patch

[youenn fablet]

Created attachment 453591 [details]
Patch

[youenn fablet]

Created attachment 453592 [details]
Patch

[youenn fablet]

Created attachment 453596 [details]
Patch

[youenn fablet]

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.

[Eric Carlson]

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`?).

[youenn fablet]

Created attachment 453828 [details]
Patch for landing

[youenn fablet]

> > 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

[youenn fablet]

Created attachment 453829 [details]
Patch for landing

[youenn fablet]

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.

[youenn fablet]

Created attachment 453846 [details]
Patch for landing - v2

[youenn fablet]

Created attachment 453946 [details]
Patch for landing - v3

[youenn fablet]

Created attachment 454101 [details]
Rebasing

[youenn fablet]

Created attachment 454102 [details]
Rebasing

[youenn fablet]

Created attachment 454111 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/90016088>

[Jon Lee]

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.