[webkit-reviews] review granted: [Bug 209365] [MSE] Handle the case where AVStreamDataParser packages sync and non-sync samples together in a CMSampleBufferRef. : [Attachment 394140] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 209365: [MSE] Handle the case where AVStreamDataParser packages sync and
non-sync samples together in a CMSampleBufferRef.
https://bugs.webkit.org/show_bug.cgi?id=209365

Attachment 394140: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 394140
  --> https://bugs.webkit.org/attachment.cgi?id=394140
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:342
> +    CFArrayRef attachmentsArray =
CMSampleBufferGetSampleAttachmentsArray(m_sample.get(), true);

The documentation says CMSampleBufferGetSampleAttachmentsArray can return NULL,
and some of our code does too (e.g. isCMSampleBufferNonDisplaying), so this
should probably check.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:344
> +    if (count == 1)

<= 1

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:369
> +    CFArrayRef attachmentsArray =
CMSampleBufferGetSampleAttachmentsArray(m_sample.get(), true);
> +    auto count = CFArrayGetCount(attachmentsArray);
> +    if (count == 1)

Ditto the comments above.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:377
> +

Nit: the extra blank isn't really needed.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:399
> +	   if (CMSampleBufferCopySampleBufferForRange(kCFAllocatorDefault,
m_sample.get(), range, &rawSample) != noErr || !rawSample)
> +	       return { };

It would be good to log an error and maybe ASSERT