Bug 237191 – Better isolate RemoteVideoFrameObjectHeap clients from ThreadSafeObjectHeap implementation details

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

[youenn fablet]

Better isolate RemoteVideoFrameObjectHeap clients from ThreadSafeObjectHeap implementation details

[youenn fablet]

Created attachment 453185 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 453185 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.h:43
> +using ThreadSafeVideoFrameHeap = ThreadSafeObjectHeap<RemoteVideoFrameIdentifier, RefPtr<WebCore::MediaSample>>;

This doesn't look useful.

If you want to make it impossible to call retire, either make the heap a member or try if it's possible to delete the individual method that you think are problematic.

> Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.h:58
> +    const WebCore::ProcessIdentity& resourceOwner() const { return m_resourceOwner; }

Don't do this.
If the resources are marked as owned when they are put to a resource heap, that is already too late.
They should be marked as owned preferably as close to creation as possible.

E.g. the fact that something is stored to the object heap does not mean the attribution is made.
The fact that GPUP creates something on behalf of WP means the attribution is made.

So if / when we want to make sure that VideoFrame instances always have attribution, we should add the process identifier as the parameter to all the classes that create VideoFrame.

[youenn fablet]

> So if / when we want to make sure that VideoFrame instances always have
> attribution, we should add the process identifier as the parameter to all
> the classes that create VideoFrame.

This is error prone as we might forget to set attribution.
AIUI, we are missing to do it in RemoteMediaPlayerProxy for instance.
I'll fix RemoteMediaPlayerProxy but I would prefer to enforce setting attribution more easily, preferably by compilation errors or through ASSERTs otherwise.

[youenn fablet]

Created attachment 453189 [details]
Patch

[EWS]

Committed r290513 (247797@main): <https://commits.webkit.org/247797@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453189 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89476575>