Bug 219812 - [Media in GPU Process][MSE] Implement SourceBuffer::reportExtraMemoryAllocated()
Summary: [Media in GPU Process][MSE] Implement SourceBuffer::reportExtraMemoryAllocated()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-11 16:17 PST by Peng Liu
Modified: 2020-12-11 22:57 PST (History)
8 users (show)

See Also:


Attachments
Patch (15.64 KB, patch)
2020-12-11 16:33 PST, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (15.60 KB, patch)
2020-12-11 16:58 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-12-11 16:17:57 PST
[Media in GPU Process][MSE] Implement SourceBuffer::reportExtraMemoryAllocated()
Comment 1 Peng Liu 2020-12-11 16:33:26 PST
Created attachment 416068 [details]
Patch
Comment 2 Eric Carlson 2020-12-11 16:39:49 PST
Comment on attachment 416068 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1194
> +void SourceBuffer::sourceBufferPrivateReportExtraMemoryCost(uint64_t extraMemory)

I don't think "sourceBufferPrivate" should be part of the method name.

> Source/WebCore/Modules/mediasource/SourceBuffer.h:161
> +    void sourceBufferPrivateReportExtraMemoryCost(uint64_t extraMemory) final;

Nit: parameter name isn't necessary.

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:624
> +    DEBUG_LOG(LOGIDENTIFIER, "currentTime = ", currentTime, ", require ", totalTrackBufferSizeInBytes() + newDataSize, " bytes, maximum buffer size is ", maximumBufferSize);
> +    uint64_t initialBufferedSize = totalTrackBufferSizeInBytes();

if you switch the order of these lines you would only need to call the method once.

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:644
> +        DEBUG_LOG(LOGIDENTIFIER, "evicted ", initialBufferedSize - totalTrackBufferSizeInBytes());

Did you mean to use the `initialBufferedSize` variable here?
Comment 3 Peng Liu 2020-12-11 16:48:11 PST
Comment on attachment 416068 [details]
Patch

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

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1194
>> +void SourceBuffer::sourceBufferPrivateReportExtraMemoryCost(uint64_t extraMemory)
> 
> I don't think "sourceBufferPrivate" should be part of the method name.

Hmm, I agree that "sourceBufferPrivate" in the method name looks strange. I choose to keep it because this will make its name consistent with other methods of SourceBufferPrivateClient.

>> Source/WebCore/Modules/mediasource/SourceBuffer.h:161
>> +    void sourceBufferPrivateReportExtraMemoryCost(uint64_t extraMemory) final;
> 
> Nit: parameter name isn't necessary.

Right! Will fix it.

>> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:624
>> +    uint64_t initialBufferedSize = totalTrackBufferSizeInBytes();
> 
> if you switch the order of these lines you would only need to call the method once.

Good point! Will fix it.

>> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:644
>> +        DEBUG_LOG(LOGIDENTIFIER, "evicted ", initialBufferedSize - totalTrackBufferSizeInBytes());
> 
> Did you mean to use the `initialBufferedSize` variable here?

Yes. It will log how much memory is freed from the track buffer.
Comment 4 Peng Liu 2020-12-11 16:58:23 PST
Created attachment 416071 [details]
Patch for landing
Comment 5 EWS 2020-12-11 22:56:54 PST
Committed r270731: <https://trac.webkit.org/changeset/270731>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416071 [details].
Comment 6 Radar WebKit Bug Importer 2020-12-11 22:57:15 PST
<rdar://problem/72250848>