[webkit-reviews] review granted: [Bug 206043] [Media in GPU process] Implement the remote video layer support : [Attachment 387446] Patch (update changelogs and fix unified build failures)

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 206043: [Media in GPU process] Implement the remote video layer support
https://bugs.webkit.org/show_bug.cgi?id=206043

Attachment 387446: Patch (update changelogs and fix unified build failures)

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




--- Comment #13 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 387446
  --> https://bugs.webkit.org/attachment.cgi?id=387446
Patch (update changelogs and fix unified build failures)

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

> Source/WebCore/platform/graphics/cv/ImageTransferSessionVT.mm:36
> +#import "CoreVideoSoftLink.h"

We usually add a line between SoftLink headers and others.

> Source/WebCore/platform/graphics/gpu/GPUSwapChainDescriptor.h:31
> +#include "GPUErrorScopes.h"

If it is a unified build issue, we usually fix this in cpp files, not. h files.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:201
> +void
RemoteMediaPlayerManagerProxy::prepareForPlayback(MediaPlayerPrivateRemoteIdent
ifier id, bool privateMode, WebCore::MediaPlayerEnums::Preload preload, bool
preservesPitch, bool prepareForRendering, WebCore::LayoutPoint&& layoutPoint,
WebCore::LayoutSize&& layoutSize, float videoContentScale,
CompletionHandler<void(LayerHostingContextID contextId)>&& completionHandler)

Probably do not need to have &&, we could probably pass them by value.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:203
>      if (auto player = m_proxies.get(id))

It would be better to call completionHandler even if there is no player.
Maybe contextId should be an Optional<>?

Maybe ASSERT(m_proxies.contains(id));

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:31
> +#include "LayerHostingContext.h"

Forward declare LayerHostingContextID?

> Source/WebKit/Platform/cocoa/LayerHostingContext.h:28
> +#include "LayerTreeContext.h"

Do we need this one?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:96
> +	   m_layerHostingContextId = contextId;

m_layerHostingContextId does not seem used anywhere.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:97
> +	   m_videoLayer =
LayerHostingContext::createPlatformLayerForHostingContext(contextId);

contextId here can be 0, is it ok.
If we are using an ObjectIdentifier, we are sure this will not be 0.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:303
> +    uint32_t m_layerHostingContextId  { 0 };

Can we use an ObjectIdentifier<>?

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:308
> +    if (auto player = m_players.get(id))

s/auto/auto&/?