Bug 237191 - Better isolate RemoteVideoFrameObjectHeap clients from ThreadSafeObjectHeap implementation details
Summary: Better isolate RemoteVideoFrameObjectHeap clients from ThreadSafeObjectHeap i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 237198
  Show dependency treegraph
 
Reported: 2022-02-25 01:14 PST by youenn fablet
Modified: 2022-02-25 08:58 PST (History)
9 users (show)

See Also:


Attachments
Patch (20.95 KB, patch)
2022-02-25 01:29 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (16.38 KB, patch)
2022-02-25 03:27 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-02-25 01:14:00 PST
Better isolate RemoteVideoFrameObjectHeap clients from ThreadSafeObjectHeap implementation details
Comment 1 youenn fablet 2022-02-25 01:29:27 PST
Created attachment 453185 [details]
Patch
Comment 2 Kimmo Kinnunen 2022-02-25 01:54:12 PST
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.
Comment 3 youenn fablet 2022-02-25 03:26:13 PST
> 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.
Comment 4 youenn fablet 2022-02-25 03:27:34 PST
Created attachment 453189 [details]
Patch
Comment 5 EWS 2022-02-25 08:57:40 PST
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].
Comment 6 Radar WebKit Bug Importer 2022-02-25 08:58:26 PST
<rdar://problem/89476575>