[webkit-reviews] review requested: [Bug 190728] [MediaStream] Allow ports to optionally do screen capture in the UI process : [Attachment 352731] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has asked  for review:
Bug 190728: [MediaStream] Allow ports to optionally do screen capture in the UI
process
https://bugs.webkit.org/show_bug.cgi?id=190728

Attachment 352731: Patch

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




--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 352731
  --> https://bugs.webkit.org/attachment.cgi?id=352731
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?