[webkit-reviews] review granted: [Bug 237594] Have MediaFormatReader plugin use WebMParser directly : [Attachment 454173] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jean-Yves Avenard [:jya]
<jean-yves.avenard at apple.com>'s request for review:
Bug 237594: Have MediaFormatReader plugin use WebMParser directly
https://bugs.webkit.org/show_bug.cgi?id=237594

Attachment 454173: Patch

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




--- Comment #8 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 454173
  --> https://bugs.webkit.org/attachment.cgi?id=454173
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.