Bug 225620 – [iPad] SourceBufferPrivateAVFObjC should not report an error to the web page when the video playback is interrupted

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

SourceBufferPrivateAVFObjC should not report an error to the web page when the video playback is interrupted

[Peng Liu]

<rdar://70118480>

[Peng Liu]

Created attachment 428214 [details]
Patch

[Darin Adler]

Comment on attachment 428214 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:971
> +#if PLATFORM(IOS_FAMILY)
> +    if ([layer status] == AVQueuedSampleBufferRenderingStatusFailed && [[error domain] isEqualToString:@"AVFoundationErrorDomain"] && [error code] == AVErrorOperationInterrupted) {
> +        m_displayLayerWasInterrupted = true;
> +        return;
> +    }
> +#endif

Why isn’t this needed on Mac? Why are we compiling all the code to handle m_displayLayerWasInterrupted on Mac, but then not using it there?

[Peng Liu]

Comment on attachment 428214 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:971
>> +#endif
> 
> Why isn’t this needed on Mac? Why are we compiling all the code to handle m_displayLayerWasInterrupted on Mac, but then not using it there?

The bug that this patch is fixing only happens on iPads. Based on my test results, AVFoundation only reports the error (AVErrorOperationInterrupted) on iPads. Therefore, I enabled this code section for iOS only.

Potentially, other code sections related to "m_displayLayerWasInterrupted" are useful for Mac, so I did not wrap them with the macro. But it seems a good idea to compile them for iOS only because they won't be used on Mac for now.

[Peng Liu]

Created attachment 428293 [details]
Make all changes for iOS only

[Jer Noble]

Comment on attachment 428293 [details]
Make all changes for iOS only

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1206
> +#if PLATFORM(IOS_FAMILY)
> +    if (m_displayLayerWasInterrupted)
> +        return false;
> +#endif

This will block appending samples to the audio renderer as well as the display layer. Consider moving this inside the `if (trackID == m_enabledVideoTrackID)` block below.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1260
> +#if PLATFORM(IOS_FAMILY)
> +    if (m_displayLayerWasInterrupted)
> +        return;
> +#endif

Ditto, and consider adding a call to `[m_displayLayer stopRequestingMediaData];` before the return.

[Peng Liu]

Comment on attachment 428293 [details]
Make all changes for iOS only

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

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1206
>> +#endif
> 
> This will block appending samples to the audio renderer as well as the display layer. Consider moving this inside the `if (trackID == m_enabledVideoTrackID)` block below.

Good point! But looks like it won't impact the function, because we will want to stop appending audio samples when video is interrupted.
I will fix it anyway.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1260
>> +#endif
> 
> Ditto, and consider adding a call to `[m_displayLayer stopRequestingMediaData];` before the return.

Hmm, probably we can just remove this change. Let me have a try.

[Peng Liu]

Created attachment 428421 [details]
Patch for landing

[Peng Liu]

Comment on attachment 428293 [details]
Make all changes for iOS only

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

>>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1260
>>> +#endif
>> 
>> Ditto, and consider adding a call to `[m_displayLayer stopRequestingMediaData];` before the return.
> 
> Hmm, probably we can just remove this change. Let me have a try.

Confirmed that this change is not needed. Removed.

[EWS]

Committed r277424 (237672@main): <https://commits.webkit.org/237672@main>

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