MediaRecorder should fire a stop event when all tracks are ended

[Wendy]

MediaRecorder should and fire stop and data available events when all tracks are ended accordion to section 2.3 of https://w3c.github.io/mediacapture-record/#mediarecorder-api

[Wendy]

Created attachment 352642 [details]
Patch

[Wendy]

Created attachment 352644 [details]
Patch

[Wendy]

Created attachment 352653 [details]
Patch

[youenn fablet]

Comment on attachment 352653 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:50
> +    return trackPrivates;

You can use WTF::map here, something like:
return WTF::map(tracks, [] (const auto& track) -> RefPtr<MediaStreamTrackPrivate> {
    return &track->privateTrack();
});
We probably want a Vector<Ref< MediaStreamTrackPrivate>> instead of Vector<RefPtr< MediaStreamTrackPrivate>>.
This might do the trick:
return WTF::map(tracks, [] (const auto& track) -> Ref<MediaStreamTrackPrivate> {
    return track->privateTrack();
});

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:43
> +    }

With a MediaStreamTrackPrivateVector&&, you would do something like:
m_tracks = WTFMove(track)
for (auto& track : m_tracks)
    track->addObserver(*this);

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:33
> +class MediaRecorderPrivate : public RefCounted<MediaRecorderPrivate>, private MediaStreamTrackPrivate::Observer {

Should probably be final.
I am not sure MediaRecorderPrivate needs to be RefCounted.
Instead MediaRecorder could take a std::unique_ptr<MediaRecorderPrivate>.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:44
> +    void removeObserver(Observer&);

No need for more than one observer.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:49
> +    MediaRecorderPrivate(const MediaStreamTrackPrivateVector&);

Should use explicit with a constructor with one parameter.
In that case, it should be a MediaStreamTrackPrivateVector&& and probably take another parameter (so no explicit anymore) for Owner (see below).

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:52
> +    void trackEnded(MediaStreamTrackPrivate&) override;

s/override/final here and below.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:57
> +    Vector<Observer*> m_observers;

Let's have just one observer which will be MediaRecorder.
And maybe rename it to Owner.
Owner & m_owner;

> Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSourceCocoa.cpp:-33
> -#include "RealtimeVideoUtilities.h"

Is this change needed?
If not, let's not do it there.

[Wendy]

Created attachment 352728 [details]
Patch

[youenn fablet]

Comment on attachment 352728 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:106
> +        auto event = Event::create(eventNames().stopEvent, Event::CanBubble::No, Event::IsCancelable::No);

Can you add a FIXME: add data available event?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:40
> +    m_owner = &owner;

m_owner should be a reference probably, not a pointer.
   m_owner(owner) should do the trick.
I would tend to pass owner first and tracks second.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:59
> +    }

You can probably use Vector::findMatching

> LayoutTests/imported/w3c/ChangeLog:9
> +        * web-platform-tests/mediacapture-record/MediaRecorder-stop.html: Added.

Please upload this test to WPT.

[Wendy]

Created attachment 352743 [details]
Patch

[Wendy]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/13606

[Wendy]

Created attachment 352744 [details]
Patch

[youenn fablet]

I looked further at the specification and think that we may better be served by changing the model a bit:
1. Make MediaRecorder an observer of all track private.
2. Handle "all ended tracks" in MediaRecorder

When implementing dataavailable event, we will then introduce a MediaRecorderPrivate method to get the available data. We might pass a function or completion handler to MediaRecorderPrivate if we need to get the data asynchronously.

The impact on this patch is to remove MediaRecorderPrivate from this patch and move part of MediaRecorderPrivate implementation of this patch in MediaRecorder.
We will then introduce MediaRecorderPrivate as part of dataavailable event implementation.

[youenn fablet]

Ms2Jer put some comments in https://github.com/web-platform-tests/wpt/pull/13606.
It might be good to update the tests accordingly.

[Wendy]

Created attachment 352789 [details]
Patch

[youenn fablet]

Comment on attachment 352789 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html:10
> +<canvas id="canvas">

Apparently, Firefox does not like capturing canvas without size.
Can you test it on Firefox and try adding width/height to see whether that would make it work if not already?

[Wendy]

Created attachment 352834 [details]
Patch

[Wendy]

(In reply to youenn fablet from comment #13)
> Comment on attachment 352789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352789&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html:10
> > +<canvas id="canvas">
> 
> Apparently, Firefox does not like capturing canvas without size.
> Can you test it on Firefox and try adding width/height to see whether that
> would make it work if not already?

I did the test on FireFox, in addition to the width and height, we might also need getContext to initialize the canvas, otherwise we will get an exception. I have fixed it in the latest patch.

[youenn fablet]

I am surprised getContex is needed, maybe this is a bug on Firefox side.
I guess we should reuse some of the WPT routines used to create a/v tracks

[WebKit Commit Bot]

Comment on attachment 352834 [details]
Patch

Clearing flags on attachment: 352834

Committed r237311: <https://trac.webkit.org/changeset/237311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45430564>

[Truitt Savell]

Looks like the test imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html

added in https://trac.webkit.org/changeset/237311/webkit

is a flakey failure. It is highly intermittent and seems to be having a harness timeout. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fmediacapture-record%2FMediaRecorder-stop.html

Diff:
--- /Volumes/Data/slave/sierra-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop-actual.txt
@@ -1,4 +1,6 @@
+
+Harness Error (TIMEOUT), message = null
 
 PASS MediaRecorder will stop recording and fire a stop event when all tracks are ended 
-PASS MediaRecorder will stop recording and fire a stop event when stop() is called 
+TIMEOUT MediaRecorder will stop recording and fire a stop event when stop() is called Test timed out