Bug 212274

Summary: MediaRecorder stopRecorder() returns empty Blob after first use
Product: WebKit Reporter: majo
Component: MediaAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, eric.carlson, ews-watchlist, glenn, hta, jer.noble, Lawrence.j, majo, philipj, sergio, tommyw, webkit-bug-importer, youennf, zach
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: macOS 10.15   
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Patch
none
Patch for relanding
none
Patch for relanding none

Description majo 2020-05-22 13:05:04 PDT
I started working on fixing this bug https://bugs.webkit.org/show_bug.cgi?id=202233 but found weird behaviour with stopRecording(). 
It returns an empty blob when you press stop after you press start, then stop and the start again.

To replicate:

 1. Open Safari
 2. Open the Develop Menu
 3. Hover over the Experimental Features list item
 4. Click on MediaRecorder to Enable it
 5. Navigate to https://codepen.io/majov5/pen/KKdEpKZ
 6. Click on the blue Start Test button at the bottom
 7. Allow access to Microphone & Camera
 8. Click on the Record button at the bottom
 9. Wait a few second and click Stop.
 
In the console of Codepen will we something like

[object Blob] {
   size: 18201,
   slice: function slice() {...},
   type: "video/mp4"
}

 10. Click on the Record button again.
 11. Wait a few second and click Stop.

Now in the console we have empty Blob

[object Blob] {
   size: 0,
   slice: function slice() {...},
   type: ""
}

Is this supposed to happen?

I have test it in Chrome and Firefox, and I always get a Blob with content.
 
I have test it in Safari 13.1, Safari Technology Preview (Release 106 (Safari 13.2, WebKit 15610.1.12.2)) and the MiniBrowser and is the same result as explained in the 11 steps.
Comment 1 Radar WebKit Bug Importer 2020-05-25 10:27:17 PDT
<rdar://problem/63601298>
Comment 2 youenn fablet 2020-06-24 05:24:43 PDT
Created attachment 402637 [details]
WIP
Comment 3 youenn fablet 2020-06-24 08:46:15 PDT
Created attachment 402653 [details]
Patch
Comment 4 Eric Carlson 2020-06-24 08:50:59 PDT
Comment on attachment 402653 [details]
Patch

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

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:192
> +    fprintf(stderr, "AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded1 %p\n", this);

Oops!

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:194
> +        fprintf(stderr, "AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded2 %p\n", this);

Ditto.

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:206
> +    fprintf(stderr, "AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded3 %p\n", this);

Ditto.

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:208
> +        fprintf(stderr, "AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded4 %p\n", this);

Ditto.
Comment 5 youenn fablet 2020-06-25 06:45:47 PDT
Created attachment 402728 [details]
Patch
Comment 6 EWS 2020-06-25 08:58:45 PDT
Committed r263511: <https://trac.webkit.org/changeset/263511>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402728 [details].
Comment 7 Chris Dumez 2020-06-25 09:56:16 PDT
Follow-up build fix:
<https://trac.webkit.org/changeset/263514>
Comment 8 Jason Lawrence 2020-06-26 14:45:31 PDT
Reverted r263511, r263514, and r263565 for reason:

r263511 caused MediaRecorder test crashes on internal testers.

Committed r263588: <https://trac.webkit.org/changeset/263588>
Comment 9 youenn fablet 2020-06-26 14:59:34 PDT
Fix is in MediaRecorderPrivateMock::fetchData to unhold m_bufferLock before calling the completion handler since calling the completion handler might delete 'this'
Comment 10 youenn fablet 2020-06-26 15:12:21 PDT
Created attachment 402911 [details]
Patch
Comment 11 EWS 2020-06-27 09:15:24 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 12 youenn fablet 2020-06-28 02:46:26 PDT
Created attachment 402988 [details]
Patch for relanding
Comment 13 EWS 2020-06-28 08:12:02 PDT
Committed r263633: <https://trac.webkit.org/changeset/263633>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402988 [details].
Comment 14 Ryan Haddad 2020-07-02 12:35:05 PDT
Reverted r263633, r263651, and r263671 for reason:

Still seeing MediaRecorder test crashes after re-landing r263633

Committed r263854: <https://trac.webkit.org/changeset/263854>
Comment 15 Ryan Haddad 2020-07-02 12:36:53 PDT
(In reply to Ryan Haddad from comment #14)
> Reverted r263633, r263651, and r263671 for reason:
> 
> Still seeing MediaRecorder test crashes after re-landing r263633
Details in radar.
Comment 16 youenn fablet 2020-07-03 02:53:13 PDT
Created attachment 403447 [details]
Patch for relanding
Comment 17 EWS 2020-07-03 05:01:47 PDT
Committed r263891: <https://trac.webkit.org/changeset/263891>

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