Bug 190728 – [MediaStream] Allow ports to optionally do screen capture in the UI process

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

[Eric Carlson]

Allow ports to optionally do screen capture in the UI process

[Radar WebKit Bug Importer]

<rdar://problem/45376824>

[Eric Carlson]

Created attachment 352731 [details]
Patch

[EWS Watchlist]

Attachment 352731 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:170:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:171:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 352731 [details]
Patch

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

> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:47
> +        WebCore::IOSurface::moveToPool(WTFMove(m_ioSurface));

You should only pool your surfaces if you're sure that everything that you use them for correctly maintains the IsInUse bits (otherwise we might re-use them while still in use). It seems likely that CV does this correctly, but it's good to be sure.

> Source/WebCore/platform/graphics/RemoteVideoSample.h:38
> +    RemoteVideoSample() = default;

Should the constructor really be public? Don't you want everyone using the ::create methods?

> Source/WebCore/platform/graphics/RemoteVideoSample.h:41
> +#if HAVE(IOSURFACE)

Or maybe not? You have no `::create`s if !HAVE(IOSURFACE), should the whole class just go away?

> Source/WebCore/platform/graphics/RemoteVideoSample.h:59
> +            encoder << WTF::MachSendRight();

This is interesting. Maybe OK to encode an empty send right, but maybe it should just be an optional instead? I'm not sure.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:400
> +    if (!size.isEmpty() && size != roundedIntSize(sample.presentationSize())) {

How did the sample size ever end up non-integer? Is rounding right, or enclosing?

[Tim Horton]

I didn't mean to reset jer's review, sorry jer.

[Eric Carlson]

Comment on attachment 352731 [details]
Patch

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

>> Source/WebCore/platform/graphics/RemoteVideoSample.cpp:47
>> +        WebCore::IOSurface::moveToPool(WTFMove(m_ioSurface));
> 
> You should only pool your surfaces if you're sure that everything that you use them for correctly maintains the IsInUse bits (otherwise we might re-use them while still in use). It seems likely that CV does this correctly, but it's good to be sure.

Removed.

>> Source/WebCore/platform/graphics/RemoteVideoSample.h:38
>> +    RemoteVideoSample() = default;
> 
> Should the constructor really be public? Don't you want everyone using the ::create methods?

Nope, made private.

>> Source/WebCore/platform/graphics/RemoteVideoSample.h:59
>> +            encoder << WTF::MachSendRight();
> 
> This is interesting. Maybe OK to encode an empty send right, but maybe it should just be an optional instead? I'm not sure.

I copied this from RemoteLayerBackingStore::encode so I'll leave it for now anyway.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:400
>> +    if (!size.isEmpty() && size != roundedIntSize(sample.presentationSize())) {
> 
> How did the sample size ever end up non-integer? Is rounding right, or enclosing?

Good point, I'll change it to expandedIntSize()

[Eric Carlson]

Created attachment 352739 [details]
Patch for landing

[Eric Carlson]

Created attachment 352740 [details]
Rebased patch for landing

[EWS Watchlist]

Attachment 352740 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:170:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:171:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 352740 [details]
Rebased patch for landing

Clearing flags on attachment: 352740

Committed r237272: <https://trac.webkit.org/changeset/237272>

[Truitt Savell]

Reverted r237272 for reason:

Broke on device iOS builds and Windows builds

Committed r237275: <https://trac.webkit.org/changeset/237275>

[Truitt Savell]

Looks like https://trac.webkit.org/changeset/237272/webkit 

Has caused iOS and Windows build failures on OpenSource 

iOS: https://build.webkit.org/builders/Apple%20iOS%2012%20Release%20%28Build%29/builds/657
Windows: https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/12538

[Eric Carlson]

Created attachment 352761 [details]
Anothher rebased patch for landing

[EWS Watchlist]

Attachment 352761 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:170:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:171:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 352763 [details]
Anothher rebased patch for landing

[EWS Watchlist]

Attachment 352763 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:170:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:171:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 352764 [details]
Anothher rebased patch for landing

[EWS Watchlist]

Attachment 352764 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:170:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:171:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 352764 [details]
Anothher rebased patch for landing

Clearing flags on attachment: 352764

Committed r237281: <https://trac.webkit.org/changeset/237281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.