Bug 237472 - Split SourceBufferParserWebM and have platform agnostic WebMParser
Summary: Split SourceBufferParserWebM and have platform agnostic WebMParser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on: 237078 237473
Blocks: 236754 237594
  Show dependency treegraph
 
Reported: 2022-03-04 06:23 PST by Jean-Yves Avenard [:jya]
Modified: 2022-03-08 18:26 PST (History)
10 users (show)

See Also:


Attachments
Patch (132.53 KB, patch)
2022-03-04 20:05 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Combined patch for EWS tests (171.28 KB, patch)
2022-03-04 20:07 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Combined patch for EWS tests (171.33 KB, patch)
2022-03-04 20:18 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Combined patch for EWS tests (176.72 KB, patch)
2022-03-04 21:39 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for review (140.05 KB, patch)
2022-03-05 19:45 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Combined Patch for EWS (178.15 KB, patch)
2022-03-05 19:46 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for review (140.89 KB, patch)
2022-03-07 02:22 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for review (140.63 KB, patch)
2022-03-07 02:30 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2022-03-04 06:23:01 PST
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.
Comment 1 Radar WebKit Bug Importer 2022-03-04 06:23:11 PST
<rdar://problem/89810969>
Comment 2 Jean-Yves Avenard [:jya] 2022-03-04 20:05:44 PST
Created attachment 453889 [details]
Patch
Comment 3 Jean-Yves Avenard [:jya] 2022-03-04 20:07:29 PST
Created attachment 453890 [details]
Combined patch for EWS tests
Comment 4 Jean-Yves Avenard [:jya] 2022-03-04 20:18:35 PST
Created attachment 453891 [details]
Combined patch for EWS tests

fix compilation TV and Watch
Comment 5 Jean-Yves Avenard [:jya] 2022-03-04 21:39:46 PST
Created attachment 453899 [details]
Combined patch for EWS tests
Comment 6 Jean-Yves Avenard [:jya] 2022-03-05 19:45:25 PST
Created attachment 453927 [details]
Patch for review
Comment 7 Jean-Yves Avenard [:jya] 2022-03-05 19:46:17 PST
Created attachment 453928 [details]
Combined Patch for EWS
Comment 8 Eric Carlson 2022-03-06 09:02:08 PST
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?
Comment 9 Jean-Yves Avenard [:jya] 2022-03-06 21:59:22 PST
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
Comment 10 Jean-Yves Avenard [:jya] 2022-03-07 02:21:29 PST
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
Comment 11 Jean-Yves Avenard [:jya] 2022-03-07 02:22:41 PST
Created attachment 453950 [details]
Patch for review

Propage error detail
Comment 12 Jean-Yves Avenard [:jya] 2022-03-07 02:30:42 PST
Created attachment 453951 [details]
Patch for review
Comment 13 Eric Carlson 2022-03-08 08:31:41 PST
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.
Comment 14 EWS 2022-03-08 18:26:37 PST
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].