Bug 214972 - MediaRecorderPrivateWriter::appendEndOfVideoSampleDurationIfNeeded should only call requestMediaDataWhenReadyOnQueue once
Summary: MediaRecorderPrivateWriter::appendEndOfVideoSampleDurationIfNeeded should onl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 85851
  Show dependency treegraph
 
Reported: 2020-07-30 09:10 PDT by youenn fablet
Modified: 2020-08-03 13:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.17 KB, patch)
2020-07-31 09:06 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (10.27 KB, patch)
2020-08-03 00:48 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-07-30 09:10:40 PDT
MediaRecorderPrivateWriter::appendEndOfVideoSampleDurationIfNeeded should only call requestMediaDataWhenReadyOnQueue once
Comment 1 youenn fablet 2020-07-31 09:06:09 PDT
Created attachment 405697 [details]
Patch
Comment 2 youenn fablet 2020-08-03 00:48:47 PDT
Created attachment 405816 [details]
Patch for landing
Comment 3 youenn fablet 2020-08-03 00:49:48 PDT
Thanks for the r+ Eric.
I changed it a bit to capture the queue of pending audio and video samples in the block given to requestMediaDataWhenReadyOnQueue. This should ensure we do not grab other incoming samples.
Comment 4 EWS 2020-08-03 03:09:44 PDT
Committed r265192: <https://trac.webkit.org/changeset/265192>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405816 [details].
Comment 5 Eric Carlson 2020-08-03 07:00:59 PDT
Comment on attachment 405816 [details]
Patch for landing

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

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:263
>      [m_audioAssetWriterInput.get() appendSampleBuffer:buffer.get()];

Shouldn't we check [m_audioAssetWriterInput isReadyForMoreMediaData] before appending?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:294
> +    m_lastVideoPresentationTime = CMSampleBufferGetPresentationTimeStamp(buffer);
> +    m_lastVideoDecodingTime = CMSampleBufferGetDecodeTimeStamp(buffer);
> +    m_hasEncodedVideoSamples = true;
> +
> +    [m_videoAssetWriterInput.get() appendSampleBuffer:buffer];

Shouldn't this check [m_videoAssetWriterInput isReadyForMoreMediaData] before appending in case, for example, appendCompressedVideoSampleBufferIfPossible filled it with pending samples?
Comment 6 youenn fablet 2020-08-03 07:44:25 PDT
Thanks, will fix that as a follow-up.

(In reply to Eric Carlson from comment #5)
> Comment on attachment 405816 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405816&action=review
> 
> > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:263
> >      [m_audioAssetWriterInput.get() appendSampleBuffer:buffer.get()];
> 
> Shouldn't we check [m_audioAssetWriterInput isReadyForMoreMediaData] before
> appending?
> 
> > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:294
> > +    m_lastVideoPresentationTime = CMSampleBufferGetPresentationTimeStamp(buffer);
> > +    m_lastVideoDecodingTime = CMSampleBufferGetDecodeTimeStamp(buffer);
> > +    m_hasEncodedVideoSamples = true;
> > +
> > +    [m_videoAssetWriterInput.get() appendSampleBuffer:buffer];
> 
> Shouldn't this check [m_videoAssetWriterInput isReadyForMoreMediaData]
> before appending in case, for example,
> appendCompressedVideoSampleBufferIfPossible filled it with pending samples?
Comment 7 youenn fablet 2020-08-03 07:45:10 PDT
https://bugs.webkit.org/show_bug.cgi?id=215081
Comment 8 youenn fablet 2020-08-03 07:53:28 PDT
We also need to make sure flushCompressedSampleBuffers is not called by stopRecording while doing work for fetchData.
Comment 9 Radar WebKit Bug Importer 2020-08-03 13:34:28 PDT
<rdar://problem/66489134>