Bug 232422 – Have RemoteMediaResource use SharedMemory

• Component: Media
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

When playing a non-mse media element, the loading gets initiated in the GPU Process and proxied to the WebContent process via a RemoteMediaResource which makes the network process download the data.

The download data gets its way back to the GPU Process where it will be copied and stored in a locally allocated buffer (ultimately a NSData). That memory allocation gets assigned to the GPU Process.
With the low 220MB limits given to the GPU Process it can easily lead to the GPU Process being killed for using too much memory.

The situation is made worse when the server doesn't support range-request; the RangeResponseGenerator will end up downloading the entire media to be played and keep it in the GPU Process.

Similar to bug 230329; we should use SharedMemory allocated in the WebContent process and pass the data to the GPU process and remove memory allocation and copies.

[Jean-Yves Avenard [:jya]]

rdar://84558835

[Jean-Yves Avenard [:jya]]

*** Bug 232163 has been marked as a duplicate of this bug. ***

[Jean-Yves Avenard [:jya]]

Created attachment 442679 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 442701 [details]
Patch

[Alex Christensen]

Comment on attachment 442701 [details]
Patch

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

Needs some polish, but I like the general direction.

> Source/WebCore/platform/SharedBuffer.cpp:470
> +    return SharedBuffer::create({

It's unclear to me which constructor this is calling.  Could you make this a little more verbose?  Why are there lambdas below?

> Source/WebCore/platform/SharedBuffer.h:272
> +    void trim(size_t);

I think rather than this, it would be nicer to have the stored size (if we can't remove it) const and make a new SharedBufferDataView that views a new subset of the bytes in the DataSegment.

> Source/WebCore/platform/SharedBuffer.h:278
> +    const size_t m_positionWithinSegment;

const is good.

> Source/WebCore/platform/SharedBuffer.h:279
> +    size_t m_endTrim { 0 };

I don't think we need to store this in memory in the SharedBufferDataView.  I think we could just give it as a parameter to SharedBufferDataView::createNSData.  Something like createNSData(size_t size = this->size()).

> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:44
> +    NSUInteger _endTrim;

I think it would make more sense to store the size rather than endTrim, which is a little less common.  Then we can assert that _position + _size <= _dataSegment.size()

> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:56
> +    virtual void dataReceived(PlatformMediaResource& platform, const uint8_t* data, int length) { dataReceived(platform, SharedBuffer::create(data, length)); }

Can we make this parameter a Span<const uint8_t>?  If it's too invasive, that could be a separate patch.

> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:141
> +            bufferView.trim(bufferView.size() - bytesFromThisViewToDeliver);

This fixes an existing issue where we would sometimes give too much data.  Great!

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:913
> +- (void)resource:(PlatformMediaResource*)resource receivedNSData:(NSData*)nsData length:(int)length

This may as well receive a RetainPtr<NSData>&& to reduce ref churn.
Also, it looks like the length parameter is now redundant.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:930
> +- (void)resource:(PlatformMediaResource*)resource receivedData:(const WebCore::SharedBufferDataView&)data length:(int)length

length is unused.

> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:100
> +    if (!sharedMemory)
> +        return;

log or debug assert?

> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:79
> +    if (!sharedMemory)
> +        return;

A debug assert or a log might be in order here.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:82
> +    memcpy(sharedMemoryPtr, data, length);

The data is probably already from a SharedBuffer.  Is there a way we can chase this higher upstream and use that instead of adding another memcpy?

[Alex Christensen]

Comment on attachment 442701 [details]
Patch

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

>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:141
>> +            bufferView.trim(bufferView.size() - bytesFromThisViewToDeliver);
> 
> This fixes an existing issue where we would sometimes give too much data.  Great!

I guess this wasn't an existing issue, but just something that was needed when changing to use SharedBuffers.

[Jean-Yves Avenard [:jya]]

Comment on attachment 442701 [details]
Patch

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

>> Source/WebCore/platform/SharedBuffer.cpp:470
>> +    return SharedBuffer::create({
> 
> It's unclear to me which constructor this is calling.  Could you make this a little more verbose?  Why are there lambdas below?

This is the DataSegment::Provider constructor.
I don't see how this can be made any clearer than that. It's how the type is defined.

>> Source/WebCore/platform/SharedBuffer.h:272
>> +    void trim(size_t);
> 
> I think rather than this, it would be nicer to have the stored size (if we can't remove it) const and make a new SharedBufferDataView that views a new subset of the bytes in the DataSegment.

So a  SharedBufferDataView constructor that takes a SharedBufferDataView and a size? What about just having a setSize method instead?

>> Source/WebCore/platform/SharedBuffer.h:279
>> +    size_t m_endTrim { 0 };
> 
> I don't think we need to store this in memory in the SharedBufferDataView.  I think we could just give it as a parameter to SharedBufferDataView::createNSData.  Something like createNSData(size_t size = this->size()).

Issue is passing that information to where the actual length is needed.
I had considered this approach, it makes for a much bigger patch.

It makes sense for a view into a memory segment to know the start offset and the length.

Providing the length at creation involves modifying SharedBuffer method, and as used by the range response generator, we don't know the size we want until after we have retrieved the DataSegment

>> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:44
>> +    NSUInteger _endTrim;
> 
> I think it would make more sense to store the size rather than endTrim, which is a little less common.  Then we can assert that _position + _size <= _dataSegment.size()

Ok

>> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:56
>> +    virtual void dataReceived(PlatformMediaResource& platform, const uint8_t* data, int length) { dataReceived(platform, SharedBuffer::create(data, length)); }
> 
> Can we make this parameter a Span<const uint8_t>?  If it's too invasive, that could be a separate patch.

As mentioned in Changelog This is tracked in bug 232424.
Need to use SharedBuffer there too

>>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:141
>>> +            bufferView.trim(bufferView.size() - bytesFromThisViewToDeliver);
>> 
>> This fixes an existing issue where we would sometimes give too much data.  Great!
> 
> I guess this wasn't an existing issue, but just something that was needed when changing to use SharedBuffers.

It wasn't an existing issue, we provided the explicit length before and a new buffer was allocated and copied into

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:79
>> +        return;
> 
> A debug assert or a log might be in order here.

This is in Shared more::allocate, there's a bug about that and is a common pattern

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:82
>> +    memcpy(sharedMemoryPtr, data, length);
> 
> The data is probably already from a SharedBuffer.  Is there a way we can chase this higher upstream and use that instead of adding another memcpy?

This is tracked in bug 232424.

The change would be very invasive, and this is a P1 bug

[Alex Christensen]

Comment on attachment 442701 [details]
Patch

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

>>> Source/WebCore/platform/SharedBuffer.cpp:470
>>> +    return SharedBuffer::create({
>> 
>> It's unclear to me which constructor this is calling.  Could you make this a little more verbose?  Why are there lambdas below?
> 
> This is the DataSegment::Provider constructor.
> I don't see how this can be made any clearer than that. It's how the type is defined.

return SharedBuffer::create(DataSegment::Provider { ...
I didn't see the create function that took a Provider because it is far away from the other create functions, and it has been added since I last touched SharedBuffer.

>>> Source/WebCore/platform/SharedBuffer.h:272
>>> +    void trim(size_t);
>> 
>> I think rather than this, it would be nicer to have the stored size (if we can't remove it) const and make a new SharedBufferDataView that views a new subset of the bytes in the DataSegment.
> 
> So a  SharedBufferDataView constructor that takes a SharedBufferDataView and a size? What about just having a setSize method instead?

Then the size wouldn't be const.  I think it's important that a view into a buffer be immutable.

[Jean-Yves Avenard [:jya]]

Comment on attachment 442701 [details]
Patch

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

>>>> Source/WebCore/platform/SharedBuffer.h:272
>>>> +    void trim(size_t);
>>> 
>>> I think rather than this, it would be nicer to have the stored size (if we can't remove it) const and make a new SharedBufferDataView that views a new subset of the bytes in the DataSegment.
>> 
>> So a  SharedBufferDataView constructor that takes a SharedBufferDataView and a size? What about just having a setSize method instead?
> 
> Then the size wouldn't be const.  I think it's important that a view into a buffer be immutable.

ok.

>>>> Source/WebCore/platform/network/cocoa/RangeResponseGenerator.mm:141
>>>> +            bufferView.trim(bufferView.size() - bytesFromThisViewToDeliver);
>>> 
>>> This fixes an existing issue where we would sometimes give too much data.  Great!
>> 
>> I guess this wasn't an existing issue, but just something that was needed when changing to use SharedBuffers.
> 
> It wasn't an existing issue, we provided the explicit length before and a new buffer was allocated and copied into

Interestingly, I had missed that the length could be different to the one in the DataSegment, and that lead to the earlier failure for LayoutTests/http/tests/security/video-cross-origin-caching.html

I find that quite interesting that it's the only test that failed.

The trim & co method came in just to cater for that particular use of the SharedBufferDataView

[Jean-Yves Avenard [:jya]]

Comment on attachment 442701 [details]
Patch

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

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:930
>> +- (void)resource:(PlatformMediaResource*)resource receivedData:(const WebCore::SharedBufferDataView&)data length:(int)length
> 
> length is unused.

hmm that method shouldn't;'t even be there. Don't know how it crawled back here.

>>> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:79
>>> +        return;
>> 
>> A debug assert or a log might be in order here.
> 
> This is in Shared more::allocate, there's a bug about that and is a common pattern

SharedMemory::allocate already has lots of release_log if it fails to allocate or map the content

[Jean-Yves Avenard [:jya]]

Created attachment 442771 [details]
Patch

[Alex Christensen]

Comment on attachment 442771 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:474
> +    return SharedBuffer::create(SharedBuffer::DataSegment::Provider {
> +        [=]() { return data; },

I think this no longer keeps the SharedBufferDataView alive in the lambda.

[Jean-Yves Avenard [:jya]]

Comment on attachment 442771 [details]
Patch

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

>> Source/WebCore/platform/SharedBuffer.cpp:474
>> +        [=]() { return data; },
> 
> I think this no longer keeps the SharedBufferDataView alive in the lambda.

You're right ; it doesn't. But it keeps a strong reference to the DataSegment instead which achieves the same. SharedBufferDataView isn't refcounted so it would have copied it instead anyway.

I found that this way it makes the code clearer to read and the implementation simpler.

[Jean-Yves Avenard [:jya]]

Created attachment 442798 [details]
Patch

fix style, fix Ref creation in SharedBufferDataView constructor

[Alex Christensen]

Comment on attachment 442798 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:450
> +    RELEASE_ASSERT(m_positionWithinSegment < m_segment->size() && m_size <= m_segment->size() - m_positionWithinSegment);

These could be two separate assertions.
Also, I think it would be ok if m_positionWithinSegment were equal to m_segment->size() as long as m_size were zero, which the second assertion would catch.

> Source/WebCore/platform/SharedBuffer.cpp:476
> +    const Ref<SharedBuffer::DataSegment> segment = m_segment;
> +    const size_t size = this->size();
> +    const uint8_t* data = this->data();
> +    return SharedBuffer::create(SharedBuffer::DataSegment::Provider {
> +        [=]() { return data; },
> +        [=]() { return size; }
> +    });

The first lambda only captures a size_t, and the second lambda only captures a const uint8_t*.  You need to explicitly capture segment.

> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:69
> +    RELEASE_ASSERT((!position || position < dataSegment.size()) && size <= dataSegment.size() - position);

separate assertions
I think the first one could also just be position <= dataSegment.size().  If position is zero, then it is definitely within range.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:144
> +@property (assign, atomic) int64_t countOfBytesReceived;

Wouldn't the ivar need to be a std::atomic<int64_t> in order for this to be true?

[Jean-Yves Avenard [:jya]]

Comment on attachment 442798 [details]
Patch

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

>> Source/WebCore/platform/SharedBuffer.cpp:450
>> +    RELEASE_ASSERT(m_positionWithinSegment < m_segment->size() && m_size <= m_segment->size() - m_positionWithinSegment);
> 
> These could be two separate assertions.
> Also, I think it would be ok if m_positionWithinSegment were equal to m_segment->size() as long as m_size were zero, which the second assertion would catch.

I do think that having positionWithinSegment == size() is okay, however this is an existing assertion and would prefer not change that.

>> Source/WebCore/platform/SharedBuffer.cpp:476
>> +    });
> 
> The first lambda only captures a size_t, and the second lambda only captures a const uint8_t*.  You need to explicitly capture segment.

definite brain fart here, thanks for spotting that

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:144
>> +@property (assign, atomic) int64_t countOfBytesReceived;
> 
> Wouldn't the ivar need to be a std::atomic<int64_t> in order for this to be true?

my understanding is that the synthesised property will be atomic.
so accessing self.countOfBytesReceived is thread-safe
accessing directly _countOfBytesReceived isn't

[Jean-Yves Avenard [:jya]]

Created attachment 442874 [details]
Patch

apply comments

[EWS]

Committed r285069 (243711@main): <https://commits.webkit.org/243711@main>

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