Bug 237594

Summary: Have MediaFormatReader plugin use WebMParser directly
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: MediaAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 237472    
Bug Blocks: 236754, 237677    
Attachments:
Description Flags
Patch for review
none
Patch for EWS
none
Patch for review
none
Patch for review
none
Patch for EWS
none
Patch
none
Patch none

Description Jean-Yves Avenard [:jya] 2022-03-08 03:59:37 PST 
Sub-task of bug 236754.

Currently, the MediaFormatReader uses a SourceBufferParserWebM to demux the webm file and build a samples table.
This unnecessarily creates MediaSampleAVFObjC object which will be immediately deleted after the needed information has been read (presentation time, decode time, flags and byte range).

This is inefficient and requires to read the entire webm file into memory before we can play it.

By using the WebM parser introduced in bug 236754, we can only export the required ByteRange and build the sample table from that without needed an intermediary MediaSampleAVFObjC.
Comment 1 Radar WebKit Bug Importer 2022-03-08 04:00:13 PST 
<rdar://problem/89960307>
Comment 2 Jean-Yves Avenard [:jya] 2022-03-08 04:09:35 PST 
Created attachment 454096 [details]
Patch for review
Comment 3 Jean-Yves Avenard [:jya] 2022-03-08 04:12:27 PST 
Created attachment 454098 [details]
Patch for EWS
Comment 4 Jean-Yves Avenard [:jya] 2022-03-08 05:12:09 PST 
Created attachment 454106 [details]
Patch for review
Comment 5 Jean-Yves Avenard [:jya] 2022-03-08 05:15:47 PST 
Created attachment 454107 [details]
Patch for review
Comment 6 Jean-Yves Avenard [:jya] 2022-03-08 05:16:21 PST 
Created attachment 454108 [details]
Patch for EWS
Comment 7 Jean-Yves Avenard [:jya] 2022-03-08 16:35:36 PST 
Created attachment 454173 [details]
Patch

Simplify flags handling
Comment 8 Eric Carlson 2022-03-08 17:33:37 PST 
Comment on attachment 454173 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:1097
> +    if (m_useByteRange)
> +        m_completeMediaSamples.append({ presentationTime, presentationTime, MediaTime(duration, presentationTime.timeScale()), MediaSample::ByteRange { metadata.position, metadata.size }, isKey ? MediaSample::SampleFlags::IsSync : MediaSample::SampleFlags::None });
> +    else
> +        m_completeMediaSamples.append({ presentationTime, presentationTime, MediaTime(duration, presentationTime.timeScale()), m_completeBlockBuffer.releaseNonNull(), isKey ? MediaSample::SampleFlags::IsSync : MediaSample::SampleFlags::None });

This would be easier to read, and less error prone if/when it is edited, if you rewrote it to set a local variable based on `m_useByteRange` and passed that to `m_completeMediaSamples.append({...`. 

Is that possible?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:1188
> +    if (m_useByteRange)
> +        m_completeMediaSamples.append({ presentationTime, MediaTime::invalidTime(), m_packetDuration, MediaSample::ByteRange { metadata.position, metadata.size }, MediaSample::SampleFlags::IsSync });
> +    else
> +        m_completeMediaSamples.append({ presentationTime, MediaTime::invalidTime(), m_packetDuration, m_completeBlockBuffer.releaseNonNull(), MediaSample::SampleFlags::IsSync });

Ditto.

> Source/WebKit/Shared/mac/MediaFormatReader/MediaFormatReader.cpp:148
> +    // TODO: why do we need a storage queue different to reader's queue?

"FIXME:" is the more idiomatic way to leave a note for future WebKit work.
Comment 9 Jean-Yves Avenard [:jya] 2022-03-08 21:57:36 PST 
Created attachment 454193 [details]
Patch

apply comments
Comment 10 EWS 2022-03-08 23:40:07 PST 
Committed r291033 (248207@main): <https://commits.webkit.org/248207@main>

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