Bug 209365 - [MSE] Handle the case where AVStreamDataParser packages sync and non-sync samples together in a CMSampleBufferRef.
Summary: [MSE] Handle the case where AVStreamDataParser packages sync and non-sync sam...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-20 15:29 PDT by Jer Noble
Modified: 2020-03-23 08:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.82 KB, patch)
2020-03-20 15:36 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (9.95 KB, patch)
2020-03-20 16:02 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2020-03-20 15:29:23 PDT
[MSE] Handle the case where AVStreamDataParser packages sync and non-sync samples together in a CMSampleBufferRef.
Comment 1 Jer Noble 2020-03-20 15:30:01 PDT
<rdar://problem/60625209>
Comment 2 Jer Noble 2020-03-20 15:36:29 PDT
Created attachment 394140 [details]
Patch
Comment 3 Eric Carlson 2020-03-20 15:57:11 PDT
Comment on attachment 394140 [details]
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
Comment 4 Jer Noble 2020-03-20 16:02:55 PDT
Created attachment 394143 [details]
Patch for landing
Comment 5 EWS 2020-03-23 08:39:00 PDT
Committed r258846: <https://trac.webkit.org/changeset/258846>

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