Bug 237472 – Split SourceBufferParserWebM and have platform agnostic WebMParser

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jean-Yves Avenard [:jya]]

Sub-task of bug 236754.

Currently, the SourceBufferParserWebM demux a webm and returned CMSampleBuffer, this is okay when used with MSE; however when used with the MediaFormatReader to play plain webm: it means we always read the entire file into memory, put each demuxed sample in a MediaSampleAVFObjC object and return the entire content to the MediaFormatReader.

The aim is for the MediaFormatReader to construct a sample table, and for this it only needs to know the offset and length of each sample within the original stream.
As such, demuxing the whole lot is less than efficient in both speed and memory usage.

By using a more specialised webm demuxer we could build exactly what the MFR needs.

In this task, we will also introduce a new type of MediaSample and Track Information that is fully platform agnostic.

[Radar WebKit Bug Importer]

<rdar://problem/89810969>

[Jean-Yves Avenard [:jya]]

Created attachment 453889 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 453890 [details]
Combined patch for EWS tests

[Jean-Yves Avenard [:jya]]

Created attachment 453891 [details]
Combined patch for EWS tests

fix compilation TV and Watch

[Jean-Yves Avenard [:jya]]

Created attachment 453899 [details]
Combined patch for EWS tests

[Jean-Yves Avenard [:jya]]

Created attachment 453927 [details]
Patch for review

[Jean-Yves Avenard [:jya]]

Created attachment 453928 [details]
Combined Patch for EWS

[Eric Carlson]

Comment on attachment 453927 [details]
Patch for review

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

> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:217
> +    if (!format)
> +        return nullptr;

It is probably worth logging an error to help when this, presumably rare, failure happens.

> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:225
> +        if (err != kCMBlockBufferNoErr || !rawBlockBuffer)
> +            return nullptr;

Ditto

> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:239
> +        if (!blockBuffer)
> +            return nullptr;

Ditto

> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:246
> +            if (err != kCMBlockBufferNoErr)
> +                return nullptr;

Ditto

> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:254
> +    if (auto err = PAL::CMSampleBufferCreateReady(kCFAllocatorDefault, completeBlockBuffers.get(), format.get(), packetSizes.size(), packetTimings.size(), packetTimings.data(), packetSizes.size(), packetSizes.data(), &rawSampleBuffer))
> +        return nullptr;

Ditto

> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:258
> +        ASSERT(attachmentsArray);

Is this failure more serious than those above? If not, should those assert too?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:566
> +    if (!m_parser)
> +        return -1;

Yuck. Maybe have this return an ExceptionOr<int> instead?

> Tools/ChangeLog:26
> +        * TestWebKitAPI/Tests/WebCore/SampleMap.cpp: Update following base class definition change.

Looks like this is in the wrong place?

[Jean-Yves Avenard [:jya]]

Comment on attachment 453927 [details]
Patch for review

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

>> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:217
>> +        return nullptr;
> 
> It is probably worth logging an error to help when this, presumably rare, failure happens.

This is a utility method. AFAIK, there’s no logging possible here. The error is logged on return

>> Source/WebCore/platform/graphics/cocoa/CMUtilities.mm:258
>> +        ASSERT(attachmentsArray);
> 
> Is this failure more serious than those above? If not, should those assert too?

This is carrying on the old code. It can’t happen unless there’s a change in behaviour in the CM method. 

We create the CM Sample and give it an array of timings etc. So there will always be an arrray there. 

The other errors indicates an OOM

>> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:566
>> +        return -1;
> 
> Yuck. Maybe have this return an ExceptionOr<int> instead?

Could return a new error code. 

Otherwise , crap in, crap out. This indicates the demuxer was called after being torn down. Which can’t happen currently

[Jean-Yves Avenard [:jya]]

Comment on attachment 453927 [details]
Patch for review

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

>> Tools/ChangeLog:26
>> +        * TestWebKitAPI/Tests/WebCore/SampleMap.cpp: Update following base class definition change.
> 
> Looks like this is in the wrong place?

what do you mean? this method is modified and that template was generated by webkit-patch

[Jean-Yves Avenard [:jya]]

Created attachment 453950 [details]
Patch for review

Propage error detail

[Jean-Yves Avenard [:jya]]

Created attachment 453951 [details]
Patch for review

[Eric Carlson]

Comment on attachment 453927 [details]
Patch for review

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

>>> Tools/ChangeLog:26
>>> +        * TestWebKitAPI/Tests/WebCore/SampleMap.cpp: Update following base class definition change.
>> 
>> Looks like this is in the wrong place?
> 
> what do you mean? this method is modified and that template was generated by webkit-patch

I meant it is in the wrong comment block.

[EWS]

Committed r291025 (248199@main): <https://commits.webkit.org/248199@main>

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