Bug 237594 - Have MediaFormatReader plugin use WebMParser directly
Summary: Have MediaFormatReader plugin use WebMParser directly
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: 237472
Blocks: 236754 237677
  Show dependency treegraph
 
Reported: 2022-03-08 03:59 PST by Jean-Yves Avenard [:jya]
Modified: 2022-03-09 14:36 PST (History)
2 users (show)

See Also:


Attachments
Patch for review (49.50 KB, patch)
2022-03-08 04:09 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for EWS (182.17 KB, patch)
2022-03-08 04:12 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for review (51.96 KB, patch)
2022-03-08 05:12 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for review (48.53 KB, patch)
2022-03-08 05:15 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for EWS (181.37 KB, patch)
2022-03-08 05:16 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (48.13 KB, patch)
2022-03-08 16:35 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (52.15 KB, patch)
2022-03-08 21:57 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-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].