Bug 192069 – Implement non-timeslice mode encoding for MediaRecorder

• Component: WebRTC
• Status: RESOLVED FIXED

[Wendy]

Implement non-timeslice mode encoding for MediaRecorder

[Wendy]

Created attachment 355845 [details]
Patch

[Wendy]

Created attachment 355846 [details]
Patch

[Wendy]

Created attachment 355851 [details]
Patch

[EWS Watchlist]

Comment on attachment 355851 [details]
Patch

Attachment 355851 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10178560

New failing tests:
http/wpt/mediarecorder/MediaRecorder-real-audio-video-dataavailable.html
http/wpt/mediarecorder/MediaRecorder-real-video-only-dataavailable.html

[EWS Watchlist]

Created attachment 355854 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 355851 [details]
Patch

Attachment 355851 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10178572

New failing tests:
http/wpt/mediarecorder/MediaRecorder-real-audio-video-dataavailable.html
http/wpt/mediarecorder/MediaRecorder-real-video-only-dataavailable.html

[EWS Watchlist]

Created attachment 355855 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Wendy]

Created attachment 355856 [details]
Patch

[Wendy]

Created attachment 355857 [details]
Patch

[Dean Jackson]

Comment on attachment 355857 [details]
Patch

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

> Source/WebCore/Sources.txt:1771
> +platform/mediarecorder/MediaRecorderPrivateReal.cpp

Do we normally call these instances "Real"? I don't see any other cases.
This should be called MediaRecorderPrivateImpl

Also usually the mock implementations go in platform/mock/ although mediastreams seems to do it a little differently.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateReal.cpp:50
> +            m_recordedVideoTrackID = videoTracks[0]->id(); // FIXME: we will need to implement support for multiple video tracks, currently we only choose the first track as the recorded track

Nit: End comment with a period.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateReal.cpp:56
> +        m_recordedAudioTrackID = audioTracks[0]->id(); // ditto

Please copy the comment, in case someone deletes the first one.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateReal.h:51
> +    mutable Lock m_bufferLock;

This doesn't seem to be used anywhere.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateReal.h:52
> +    UniqueRef<MediaRecorderPrivateWriter> m_writer;

Why does this need to be a unique ref rather than just a simple instance? It's currently constructed with makeUniqueRef<MediaRecorderPrivateWriter>() -- will it ever be constructed with different parameters?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.h:53
> +public:
> +    bool setupWriter();

Should there be a MediaRecorderPrivateWriter() = default;?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.h:60
> +    Ref<Blob> fetchData();
> +private:    

Nit: Put a blank line between these two.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.h:70
> +    BinarySemaphore m_FinishWritingSemaphore;
> +    BinarySemaphore m_FinishWritingAudioSemaphore;
> +    BinarySemaphore m_FinishWritingVideoSemaphore;

These should have lowercase f after the _.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:1
> +/*

This looks like a Mac-specific implementation, so the file should probably be called MediaRecorderPrivateWriterMac.mm (or AVFoundation, or Cocoa), and maybe in a sub-directory?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

2018

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:84
> +    NSString *path = [directory stringByAppendingString:@"/test.mp4"];

This seems like a weird name to use for all output - even temporarily. Also, if there are ever more than one MediaRecorderPrivateWriters, they will clash.
I think you should generate a random name for each instance.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:99
> +    m_videoInput = adoptNS([allocAVAssetWriterInputInstance() initWithMediaType:AVMediaTypeVideo outputSettings:videoSettings sourceFormatHint:NULL]);

NULL -> nil

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:115
> +    m_audioInput = adoptNS([allocAVAssetWriterInputInstance() initWithMediaType:AVMediaTypeAudio outputSettings:audioSettings sourceFormatHint:NULL]);

NULL -> nil

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:180
> +    auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);

Don't you need to check that description.platformDescription().description isn't the nullptr variant type?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateWriter.mm:184
> +            // audio-only recording

Nit: Comment should end with a period.

> LayoutTests/http/wpt/mediarecorder/MediaRecorder-real-audio-only-dataavailable.html:44
> +        }, 5000);

Do we really need this test to run for 5 seconds?

> LayoutTests/http/wpt/mediarecorder/MediaRecorder-real-audio-video-dataavailable.html:84
> +            player.onended = () => {
> +                const resFrame = document.getElementById("frame");
> +                const resContext = resFrame.getContext('2d');
> +                resContext.drawImage(player, 0, 0);
> +                _assertPixelApprox(resFrame, 0, 0, 255, 0, 0, 255, "0, 0", "255, 0, 0, 255", 5);
> +                _assertPixelApprox(resFrame, 50, 50, 255, 0, 0, 255, "50, 50", "255, 0, 0, 255", 5);
> +                _assertPixelApprox(resFrame, 199, 0, 0, 255, 0, 255, "199, 0", "0, 255, 0, 255", 5);
> +                _assertPixelApprox(resFrame, 199, 199, 0, 255, 0, 255, "199, 199", "0, 255, 0, 255", 5);
> +                t.done();
> +            };

This tests that the last frame was correct. Maybe there should be a test for a frame in the middle (with a different canvas image).

[Wendy]

Created attachment 355945 [details]
Patch

[Wendy]

Created attachment 355954 [details]
Patch

[youenn fablet]

Comment on attachment 355954 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:49
> +UniqueRef<MediaRecorderPrivate> MediaRecorder::getPrivateImpl(Ref<MediaStream>&& stream)

It seems it could be a free function.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:51
> +    if (DeprecatedGlobalSettings::mockCaptureDevicesEnabled())

I do not think we should tie it to this settings.
Instead, we could add a new Internals method specifically for MediaRecorder.
Default value would use the real recorder private.

If we do not want to ship MediaRecorderPrivateMock in WebCore, we should compile it only with WebCoreTesting and activate it through Internals.
In that case, maybe using a function pointer that could override the default implementation would be the best option.

We compile mock capture sources as we are shipping these in Safari.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:54
> +    return makeUniqueRef<MediaRecorderPrivateAVImpl>(WTFMove(stream));

This code is cocoa specific so should probably be protected by #if.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:61
> +    , m_private(getPrivateImpl(m_stream.copyRef()))

We usually put private implementations in Platform if possible.
That would forbid to pass a MediaStream here.
Maybe we can try not passing a MediaStream?

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:109
> +    m_private->stopRecording();

This should probably be done in stopRecordingInternal() after we removed all the observers.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:41
> +    : m_stream(WTFMove(stream))

m_stream does not seem to be used elsewhere so maybe we do not need to have m_stream.
Also, instead of Ref<MediaStream>&&, we could try passing a const MediaStreamPrivate&.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:51
> +class MediaRecorderPrivateWriter : public ThreadSafeRefCounted<MediaRecorderPrivateWriter>, public CanMakeWeakPtr<MediaRecorderPrivateWriter> {

Does it need to be ThreadSafeRefCounted? Are we refing it in another thread?
Maybe we should set DestructionThread::Main if we want it to be destroyed in the main thread.

If the issue is that appendVideoSampleBuffer/appendAudioSampleBuffer are called in other threads, we should be able to remove observers that trigger calling these methods from the main thread.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:53
> +    MediaRecorderPrivateWriter() = default;

I would put a line between constructor and below methods.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:75
> +    bool firstAudioSample { true };

All these 3 members should be preceded with m_.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:139
> +                    return;

weakThis are not thread safe. Nothing prevents another thread which is the last one to dereference after the weakThis check.
Maybe the lambda should ref this to protect it.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:168
> +    free(pInfo);

I would wrap all these lines in a utility function called copySampleBufferWithDifferentPresentationTimeStamp for instance.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:171
> +        return;

If isStopped is true, it seems we should exit earlier than here.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:178
> +        return;

Should we have the same check in MediaRecorderPrivateWriter::appendVideoSampleBuffer?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:255
> +    m_finishWritingSemaphore.wait();

Do we need to wait for m_finishWritingSemaphore?
Maybe we should wait for it in fetchData instead.
I guess we have no choice than doing that since we want to set the state, send the blob event and the stop event synchronously :(

Or we could untie track observing from m_state.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:263
> +    RefPtr<SharedBuffer> data = SharedBuffer::createWithContentsOfFile(m_path);

s/RefPtr<SharedBuffer>/auto/

> LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-only-dataavailable.html:44
> +        }, 2000);

Can we reduce to less than 2 seconds?
Maybe 500 ms would be sufficient to get data.

[Wendy]

Created attachment 356110 [details]
Patch

[Wendy]

Created attachment 356112 [details]
Patch

[Wendy]

Created attachment 356113 [details]
Patch

[Wendy]

Created attachment 356118 [details]
Patch

[youenn fablet]

Comment on attachment 356113 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:65
> +    return makeUniqueRef<MediaRecorderPrivateMock>();

We should remove MediaRecorderPrivateMock from here.
I feel like getPrivateImpl should return a std::unique_ptr<MediaRecorderPrivate>.
If null, MediaRecorder::create would throw an exception as not implemented.
Could best be done as a preliminary patch.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:73
> +    static creatorFunction m_customCreator;

Should be private.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:78
> +    UniqueRef<MediaRecorderPrivate> getPrivateImpl(const MediaStreamPrivate&);

could be static.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:32
> +#include "Blob.h"

Blob is not in WebCore/platform.
It seems we should be modifying MediaRecorderPrivate::fetchData to return a SharedBuffer and a mime type so that MediaRecorder would create the Blob itself. 
Something like:
virtual RefPtr<SharedBuffer> fetchData();
virtual const String& mimeType();

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:46
> +    for (auto& track : tracks) {

To ease reading, I would write it like this:
if (!track->enabled() || track->ended())
    continue;
switch(track->type()) {
....
}

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:50
> +                m_writer.setVideoInput(settings.width(), settings.height());

We are not using setVideoInput return value.
Maybe add a FIXME so that we do so and we could then throw a JavaScript error somewhere.
That might mean we should probably have a
static std::unique_ptr<MediaRecorderPrivateAVImpl> MediaRecorderPrivateAVImpl::create(const MediaStreamPrivate& stream)

If setVideoInput/setAudioInput is failing, we would return nullptr.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:51
> +                m_recordedVideoTrackID = track->id(); // FIXME: we will need to implement support for multiple video tracks, currently we only choose the first track as the recorded track.

FIXME could have their own lines.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.h:34
> +class Blob;

Probably not needed.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.h:39
> +    MediaRecorderPrivateAVImpl(const MediaStreamPrivate&);

explicit.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:50
> +class MediaRecorderPrivateWriter : public ThreadSafeRefCounted<MediaRecorderPrivateWriter> {

Should be DestructionThread::Main

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:60
> +    Ref<Blob> fetchData();

Should be RefPtr<SharedBuffer> fetchData();

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:92
> +        return false;

We might want to RELEASE_LOG_ERROR the error case.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:99
> +        return false;

Can there be a case where m_videoInput is not null?
Should we instead just do:
ASSERT(m_videoInput);

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:105
> +        return false;

Maybe we should log this error case as well.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:121
> +        return false;

Ditto here.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:130
> +        return;

If we do not create a MediaRecorderPrivateAVImpl if writer setup fails, we could just write
ASSERT(m_videoInput);

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:132
> +    if (!m_canWrite) {

Would m_hasStartedWriting be a better name?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:134
> +        if (!success)

Can be written as if ([m_writer startWriting])?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:135
> +            m_isStopped = true;

Should we return if m_isStopped is true?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:137
> +        [m_writer startSessionAtSourceTime:startTime];

The above two lines can be written as one.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:147
> +                auto buf = m_videoBufferPool.takeFirst();

s/buf/buffer

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:164
> +CMSampleBufferRef MediaRecorderPrivateWriter::copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)

That could probably be a free function:
static inline CMSampleBufferRef copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:169
> +    CMSampleTimingInfo* pInfo = new CMSampleTimingInfo[count];

I would use a Vector<CMSampleTimingInfo> for this and maybe use Vector::fill.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:175
> +    CMSampleBufferRef sout;

s/sout/newBuffer/

[Wendy]

> Can there be a case where m_videoInput is not null?
> Should we instead just do:
> ASSERT(m_videoInput);

The if (m_videoInput) is used to guarantee only one video input and one audio input can be added to the AVAssetWriter since we only support one video track and one audio track recording now.

we will need to support multiple tracks recording.
if m_videoInput is not null, it means there AVAssetWriter has contained a video input.

[Wendy]

Created attachment 356234 [details]
Patch

[Wendy]

Created attachment 356237 [details]
Patch

[Wendy]

Created attachment 356244 [details]
Patch

[youenn fablet]

Comment on attachment 356244 [details]
Patch

LGTM, some more comments below.
Eric, would you be able to review it as well, I am not very familiar with the ObjC API.

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:51
> +        return Exception { NotSupportedError, "The MediaRecorder of audio and video source is only supported on iOS and macOS"_s };

I would change the message to "MediaRecorder is unsupported in this platform".
Ideally, we should not expose MediaRecorder in other platforms than iOS/MacOS.
Let's do that in a follow-up patch.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:149
> +Ref<Blob> MediaRecorder::createBlobByRecordingData()

s/createBlobByRecordingData/createRecordingDataBlob/

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:48
>      virtual ~MediaRecorderPrivate() = default;

I would put this one as the first line of MediaRecorderPrivate.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:64
> +            m_writer.setVideoInput(settings.width(), settings.height());

If setVideoInput fails, we should probably fail so that MediaRecorderPrivateAVImpl::create returns nullptr.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:74
> +            m_writer.setAudioInput();

If setAudioInput fails, we should probably fail so that MediaRecorderPrivateAVImpl::create returns nullptr.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:114
> +    return mp4MimeType; // FIXME: we will need to support more MIME types.

I would put the FIXME in its own line.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.h:39
> +    explicit MediaRecorderPrivateAVImpl(const MediaStreamPrivate&);

This constructor should be private.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:57
> +    bool isWriterReady() { return m_writer; }

Should be const.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:80
> +        return false;

I would either ASSERT(!m_writer) or put this code in MediaRecorderPrivateWriter constructor that would become private and add a static create method.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:91
> +        RELEASE_LOG_ERROR(Media, "create AVAssetWriter instance failed with error code %ld", (long)error.code);

I wonder whether it should use MediaStream logging.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:100
> +    if (m_videoInput)

I would ASSERT(!m_videoInput) here.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:118
> +    if (m_audioInput)

I would ASSERT(!m_audioInput) here.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:155
> +{

I would ASSERT(m_videoInput)

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:160
> +        if (![m_writer startWriting]) {

Is it an error case, should we log it?

[Eric Carlson]

Comment on attachment 356244 [details]
Patch

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

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVImpl.cpp:27
> +#include "MediaRecorderPrivateAVImpl.h"

Nit: these files should be called MediaRecorderPrivateAVFImpl (missing the "F").

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:165
> +        CMTime startTime = CMClockGetTime(CMClockGetHostTimeClock());
> +        [m_writer startSessionAtSourceTime:startTime];

Nit: you don't need the temporary "startTime" variable here.

[Wendy]

Created attachment 356405 [details]
Patch

[youenn fablet]

Comment on attachment 356405 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:39
>  #include "MediaRecorderPrivateMock.h"

"MediaRecorderPrivateMock.h" is probably not needed anymore.

We usually put COCOA specific includes at the end of the list, separated by a line.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:75
> +    , m_private(MediaRecorder::getPrivateImpl(m_stream->privateStream()))

As of a follow-up patch, we might want to create the MediaRecorderPrivate first in MediaRecorder::create and then only create MediaRecorder if we have a valid private.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:41
> +    auto AVImpl = std::unique_ptr<MediaRecorderPrivateAVFImpl>(new MediaRecorderPrivateAVFImpl(stream));

s/AVImpl/instance

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:42
> +    if (!AVImpl->isWriterReady)

s/isWriterReady/m_isWriterReady/

[Wendy]

Created attachment 356411 [details]
Patch

[Wendy]

Created attachment 356419 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356419 [details]
Patch

Clearing flags on attachment: 356419

Committed r238844: <https://trac.webkit.org/changeset/238844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46443290>

[Ryan Haddad]

This test is a flaky failure on iOS and macOS bots:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

--- /Volumes/Data/slave/mojave-debug-tests-wk1/build/layout-test-results/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-expected.txt
+++ /Volumes/Data/slave/mojave-debug-tests-wk1/build/layout-test-results/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-actual.txt
@@ -1,4 +1,7 @@
+CONSOLE MESSAGE: line 2659: Error: assert_approx_equals: Red channel of the pixel at (199, 0) expected 0 +/- 5 but got 254
 
 
-PASS MediaRecorder can successfully record the video for a audio-video stream 
+Harness Error (FAIL), message = Error: assert_approx_equals: Red channel of the pixel at (199, 0) expected 0 +/- 5 but got 254
 
+TIMEOUT MediaRecorder can successfully record the video for a audio-video stream Test timed out
+


This test is a frequent failure on macOS bots:
http/wpt/mediarecorder/MediaRecorder-AV-video-only-dataavailable.html

--- /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/http/wpt/mediarecorder/MediaRecorder-AV-video-only-dataavailable-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/http/wpt/mediarecorder/MediaRecorder-AV-video-only-dataavailable-actual.txt
@@ -1,4 +1,7 @@
+CONSOLE MESSAGE: line 2659: Error: assert_approx_equals: Red channel of the pixel at (199, 0) expected 0 +/- 5 but got 254
 
 
-PASS MediaRecorder can successfully record the video for a video-only stream 
+Harness Error (FAIL), message = Error: assert_approx_equals: Red channel of the pixel at (199, 0) expected 0 +/- 5 but got 254
 
+TIMEOUT MediaRecorder can successfully record the video for a video-only stream Test timed out
+


This test has crashed multiple times on Mojave bots:
http/wpt/mediarecorder/MediaRecorder-AV-audio-only-dataavailable.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000003d0002813 WTFCrashWithInfo(int, char const*, char const*, int) + 19 (Assertions.h:554)
1   com.apple.WebCore             	0x00000003d03e8120 initAVEncoderBitRatePerChannelKey() + 128 (MediaRecorderPrivateWriterCocoa.mm:54)
2   com.apple.WebCore             	0x00000003d03e4486 WebCore::MediaRecorderPrivateWriter::setAudioInput() + 38 (MediaRecorderPrivateWriterCocoa.mm:117)
3   com.apple.WebCore             	0x00000003d12fa261 WebCore::MediaRecorderPrivateAVFImpl::MediaRecorderPrivateAVFImpl(WebCore::MediaStreamPrivate const&) + 433 (MediaRecorderPrivateAVFImpl.cpp:75)
4   com.apple.WebCore             	0x00000003d12fa078 WebCore::MediaRecorderPrivateAVFImpl::create(WebCore::MediaStreamPrivate const&) + 40 (MediaRecorderPrivateAVFImpl.cpp:41)
5   com.apple.WebCore             	0x00000003d0938b40 WebCore::MediaRecorder::create(WebCore::Document&, WTF::Ref<WebCore::MediaStream, WTF::DumbPtrTraits<WebCore::MediaStream> >&&, WebCore::MediaRecorder::Options&&) + 128 (MediaRecorder.cpp:68)
6   com.apple.WebCore             	0x00000003d051156f WebCore::JSDOMConstructor<WebCore::JSMediaRecorder>::construct(JSC::ExecState*) + 271 (Ref.h:59)
7   com.apple.JavaScriptCore      	0x00000003d4db3ad1 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 385 (LLIntSlowPaths.cpp:1478)

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-AV

[Ryan Haddad]

https://bugs.webkit.org/show_bug.cgi?id=192371 was filed to address test flakiness, but there are still issues after the change.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 192414

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #29)
> Comment on attachment 356419 [details]
> Patch
> 
> Clearing flags on attachment: 356419
> 
> Committed r238844: <https://trac.webkit.org/changeset/238844>

https://trac.webkit.org/changeset/238846/webkit was landed as a build fix after this.

[Ryan Haddad]

*** Bug 192418 has been marked as a duplicate of this bug. ***

[Wendy]

Created attachment 356897 [details]
Patch

[Wendy]

Created attachment 356898 [details]
Patch

[Wendy]

Created attachment 356899 [details]
Patch

[EWS Watchlist]

Comment on attachment 356899 [details]
Patch

Attachment 356899 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10323106

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 356906 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Wendy]

Created attachment 356932 [details]
Patch

[Wendy]

Created attachment 356936 [details]
Patch

[youenn fablet]

Comment on attachment 356936 [details]
Patch

LGTM, modulo the potential memory leakage.

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

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:200
> +        [m_videoInput requestMediaDataWhenReadyOnQueue:m_videoPullQueue usingBlock:[this, protectedThis] {

You could do: protectedThis = WTFMove(protectedThis)

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:292
> +                return;

If we no longer have a weak pointer, we do not do this cleanup.
Part of it might be needed as a clean-up step though.
From what I see, we need to call dispatch_release for the audio and video queue otherwise we will leak memory.

I would add a method MediaRecorderPrivateWriter::clear that does this stuff and call it in the destructor.
We can call it here as well if we think that we can reuse the writer.
In that case, we need to make sure we do not call dispatch_release twice.
Probably, we can write the clear method as follows:

if (m_videoInput) {
    m_videoInput.clear();
    dispatch_release(m_videoPullQueue);
}
if (m_audioInput) {
    m_audioInput.clear();
    dispatch_release(m_audioPullQueue);
}

Another way to make sure m_videoInput and m_videoPullQueue remains in sync is to have two methods:
setVideoInput(): takes a video input, stores it into m_videoInput and create a dispatch queue
clearVideoInput(): exit early if video input is null. Otherwise clear video input and release dispatch queue.

Ditto for audioInput.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:298
> +                m_videoInput = nullptr;

clear and setting to nullptr is probably redundant.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:303
> +                m_audioInput = nullptr;

Ditto.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:307
> +            m_writer = nullptr;

Ditto.

[Wendy]

Created attachment 357196 [details]
Patch

[Wendy]

Created attachment 357198 [details]
Patch

[youenn fablet]

Comment on attachment 357198 [details]
Patch

Let's go then!
Some further improvements inline that could be handled as a follow-up patch.

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

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:104
> +    if (m_writer)

No need for m_writer check.
In some other code paths, m_writer = nullptr is used for the same purpose.
We should be consistent everywhere.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:307
> +        callOnMainThread([this, weakPtr] {

It would be better to have: weakPtr = WTFMove(weakPtr).

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:310
> +            m_isStopped = false;

It is somehow strange to have m_isStopped to false now that we actually stopped recording.
Is there a reason for this line?
Maybe this should be called m_isStopping although other places are using it as m_isStopped.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:320
> +    if ((m_path.isEmpty() && !m_isStopped) || !m_hasStartedWriting)

When we execute the callOnMainThread lambda in stopRecording(), m_hasStartedWriting will be set to false and fetchData will return nullptr. This seems ok now as we call fetchData synchronously to stopRecording() but this assumption could be broken in the future.
We should think of how to improve this.

One potentially way of thinking about this is to try triggering some actions on MediaRecorder when the stopRecording background task is finished. That might allow removing the finishWriting semaphore.

[WebKit Commit Bot]

Comment on attachment 357198 [details]
Patch

Clearing flags on attachment: 357198

Committed r239145: <https://trac.webkit.org/changeset/239145>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This broke Mac builds. e.g.
https://build.webkit.org/builders/Apple%20Mojave%20Debug%20%28Build%29/builds/1814

./platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:57:19: error: use of undeclared identifier 'MockRealtimeMediaSourceCenter'
    auto device = MockRealtimeMediaSourceCenter::mockDeviceWithPersistentID(deviceID);

[Ryosuke Niwa]

Build fix attempt landed in https://trac.webkit.org/changeset/239147.

[Shawn Roberts]

Test is still a flaky crash in Mac WK2

http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

Probable Cause:

Test appears to have been flaky on inception. 

Patch attempted, but issue persists in Mac WK2

Flakiness Dashboard:

http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-AV-audio-video-dataavailable.html

Crash Log:

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r240666%20(9151)/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-crash-log.txt