Bug 235954 - SampleBufferDiplayLayer should not need to create IOSurfaces
Summary: SampleBufferDiplayLayer should not need to create IOSurfaces
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: 235951
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-01 08:15 PST by youenn fablet
Modified: 2022-02-03 02:14 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.43 KB, patch)
2022-02-01 08:21 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-01 08:15:46 PST
SampleBufferDiplayLayer should not need to create IOSurfaces
Comment 1 youenn fablet 2022-02-01 08:21:34 PST
Created attachment 450531 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2022-02-01 08:40:24 PST
<rdar://problem/88329675>
Comment 3 Eric Carlson 2022-02-02 09:08:18 PST
Comment on attachment 450531 [details]
Patch

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

> Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:125
> +        sample = MediaSampleAVFObjC::createImageSample(WTFMove(pixelBuffer), remoteSample.rotation(), remoteSample.mirrored());
> +        sample->setTimestamps(remoteSample.time(), MediaTime { });

As a followup, it would be nice if createImageSample that took optional timestamps.

> Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:172
> +void RemoteSampleBufferDisplayLayer::setSharedVideoFrameSemaphore(IPC::Semaphore&& semaphore)
> +{
> +    m_sharedVideoFrameReader.setSemaphore(WTFMove(semaphore));
> +}
> +
> +void RemoteSampleBufferDisplayLayer::setSharedVideoFrameMemory(const SharedMemory::IPCHandle& ipcHandle)
> +{
> +    auto memory = SharedMemory::map(ipcHandle.handle, SharedMemory::Protection::ReadOnly);
> +    if (!memory)
> +        return;
> +
> +    m_sharedVideoFrameReader.setSharedMemory(memory.releaseNonNull());
> +}

And it would be nice to put these into a "mixin" class with a pure virtual `SharedVideoFrameReader` getter others can derive from instead of duplicating in all of the classes that need them.
Comment 4 youenn fablet 2022-02-03 01:53:40 PST
(In reply to Eric Carlson from comment #3)
> Comment on attachment 450531 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450531&action=review
> 
> > Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:125
> > +        sample = MediaSampleAVFObjC::createImageSample(WTFMove(pixelBuffer), remoteSample.rotation(), remoteSample.mirrored());
> > +        sample->setTimestamps(remoteSample.time(), MediaTime { });
> 
> As a followup, it would be nice if createImageSample that took optional
> timestamps.

I filed https://bugs.webkit.org/show_bug.cgi?id=236067.

> > Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:172
> > +void RemoteSampleBufferDisplayLayer::setSharedVideoFrameSemaphore(IPC::Semaphore&& semaphore)
> > +{
> > +    m_sharedVideoFrameReader.setSemaphore(WTFMove(semaphore));
> > +}
> > +
> > +void RemoteSampleBufferDisplayLayer::setSharedVideoFrameMemory(const SharedMemory::IPCHandle& ipcHandle)
> > +{
> > +    auto memory = SharedMemory::map(ipcHandle.handle, SharedMemory::Protection::ReadOnly);
> > +    if (!memory)
> > +        return;
> > +
> > +    m_sharedVideoFrameReader.setSharedMemory(memory.releaseNonNull());
> > +}
> 
> And it would be nice to put these into a "mixin" class with a pure virtual
> `SharedVideoFrameReader` getter others can derive from instead of
> duplicating in all of the classes that need them.

I can at least make setSharedMemory take a IPCHandle to centralise more code.
I filed https://bugs.webkit.org/show_bug.cgi?id=236068.
Comment 5 EWS 2022-02-03 02:14:54 PST
Committed r289038 (246745@main): <https://commits.webkit.org/246745@main>

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