MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop recording it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().

MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.

LayoutTests need the WebMediaStream to have at least an empty
content/MediaStream, so  test_runner/web_test_delegate needs to
be updated to rely on a method AddMediaStream implemented in
content_shell's BlinkTestRunner to use the content/public/renderer
MediaStreamApi :)


BUG=545156

[mcasas, 2015-12-21 20:48:48]

Description was changed from

==========
MediaRecorder: make Handler an MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop doing it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().
MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.
Finally, test_runner needs to be update to have a real, yet empty,
content MediaStream hanging from the LayoutTests WebMediaStream.

BUG=545156
==========

to

==========
MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop recording it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().
MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.
Finally, test_runner needs to be update to have a real, yet empty,
content MediaStream hanging from the LayoutTests WebMediaStream.

BUG=545156
==========

[mcasas, 2015-12-21 20:49:26]

Description was changed from

==========
MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop recording it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().
MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.
Finally, test_runner needs to be update to have a real, yet empty,
content MediaStream hanging from the LayoutTests WebMediaStream.

BUG=545156
==========

to

==========
MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop recording it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().

MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.

Finally, test_runner needs to be updated to have a real, yet empty,
content MediaStream hanging from the LayoutTests' WebMediaStream.

BUG=545156
==========

[mcasas, 2015-12-21 23:58:16]

mcasas@chromium.org changed reviewers:
+ perkj@chromium.org, tommi@chromium.org

[mcasas, 2015-12-21 23:58:17]

tommi@/perkj@ PTAL at
content/renderer/media/* files

(you're more than welcome to PTAL at the rest, too)

[mcasas, 2016-01-07 08:40:21]

On 2015/12/21 23:58:17, mcasas wrote:
> tommi@/perkj@ PTAL at
> content/renderer/media/* files
> 
> (you're more than welcome to PTAL at the rest, too)

Welcome back and ping :)

[mcasas, 2016-01-07 08:40:24]

On 2015/12/21 23:58:17, mcasas wrote:
> tommi@/perkj@ PTAL at
> content/renderer/media/* files
> 
> (you're more than welcome to PTAL at the rest, too)

Welcome back and ping :)

[perkj_chrome, 2016-01-07 09:18:00]

It looks like this cl stop the MediaStream if  a track is added or removed. That
might make sence. 

But the referenced bug talk about that recording should stop if a track is
stopped. That it not the same thing as adding or removing tracks in the
MediaStream.

https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_user_media_client.cc (right):

https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_user_media_client.cc:166:
delegate_->AddMediaStream(&stream);
this seems wrong. Why not just do stream.setExtraData(new
content::MediaStream()) if you need this.

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_api.cc:133: CONTENT_EXPORT void
AddMediaStreamTowebMediaStream(
? This seems wrong. In production , this happens when the MediaStream is
created. If the MediaStream is created by blink, this is triggered. 

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_api.h (right):

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_api.h:53: blink::WebMediaStream*
web_stream);
?

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:100:
DCHECK(MediaStream::GetMediaStream(media_stream));
no need for this check. The same will happen on the line below if
MediaStream::GetMediaStream return null.

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:209:
WriteData(base::StringPiece(""));
Why is WriteData(base::StringPiece necessary here but not in the normal stop
condition? If it is required always, move to stop() ?

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream.cc:23: observers_.clear();
Why this change? Is there a case when the observers can outlive the MediaStream?
It that case, how do they know that they can not call RemoveObserver ?

[perkj_chrome, 2016-01-07 09:22:59]

perkj@chromium.org changed reviewers:
+ emircan@chromium.org

[perkj_chrome, 2016-01-07 09:22:59]

Also adding emircan as a reviewer. 
Please also add a test for this. Do you have any layout tests or content
browsertests ?

[mcasas, 2016-01-08 00:03:52]

Description was changed from

==========
MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop recording it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().

MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.

Finally, test_runner needs to be updated to have a real, yet empty,
content MediaStream hanging from the LayoutTests' WebMediaStream.

BUG=545156
==========

to

==========
MediaRecorder: make MediaRecorderHandler a MediaStreamObserver

MediaRecorderHandler should observe the MediaStream being recorded and
stop recording it if the amount of Tracks varies. For that purpose this CL
makes MRH a MediaStreamObserver and registers itself during initialize().

MS expects currently that the observer de-register themselves before
~MS(), but that seem to prohibitively peg Observer and Observee lifetime,
so that condition is removed.

LayoutTests need the WebMediaStream to have at least an empty
content/MediaStream, so  test_runner/web_test_delegate needs to
be updated to rely on a method AddMediaStream implemented in
content_shell's BlinkTestRunner to use the content/public/renderer
MediaStreamApi :)


BUG=545156
==========

[mcasas, 2016-01-08 00:14:11]

mcasas@chromium.org changed reviewers:
+ jochen@chromium.org

[mcasas, 2016-01-08 00:14:12]

perkj@ PTAL

jochen@: blessing of the small mod in:
- components/test_runner/web_test_delegate.h
impl in
- content/shell/renderer/layout_test/blink_test_runner.{cc,h}
- components/html_viewer/web_test_delegate_impl.{cc,h}
and
- components/test_runner/mock_web_user_media_client.cc
(see the description for context).

https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_user_media_client.cc (right):

https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_user_media_client.cc:166:
delegate_->AddMediaStream(&stream);
On 2016/01/07 09:18:00, perkj_chrome wrote:
> this seems wrong. Why not just do stream.setExtraData(new
> content::MediaStream()) if you need this.

The gist of this is that test_runner cannot depend on 
content/ [1]. 

(Instead, the idea is to add stuff to a WebTestDelegate  
and implement that accordingly).


[1] https://codereview.chromium.org/1371373002/#msg37

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_api.cc:133: CONTENT_EXPORT void
AddMediaStreamTowebMediaStream(
On 2016/01/07 09:18:00, perkj_chrome wrote:
> ? This seems wrong. In production , this happens when the MediaStream is
> created. If the MediaStream is created by blink, this is triggered. 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Yes, this is just for content_shell and is only invoked
in blink_test_runner.cc., see my comments elsewhere
as to the need for this method.

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_api.h (right):

https://codereview.chromium.org/1543673002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_api.h:53: blink::WebMediaStream*
web_stream);
On 2016/01/07 09:18:00, perkj_chrome wrote:
> ?

Following the previous explanation, WebTestDelegate is
implemented in 
content/shell/renderer/layout_tests/blink_test_runner.h,
that can only depend on content/public. And I don't
think MediaStream class should be there, instead the
pattern followed is to use media_stream_api.{cc,h} to
"implement" the calls from the WebTestDelegate in a
correct fashion (e.g.
BlinkTestRunner::AddMediaStreamVideoSourceAndTrack() 
etc).

So WebTestDelegate --> BlinkTestRunner --...
-->content/public/renderer/media_stream_api --...
--> content/renderer/media/ real stuff.

But fret not, since this CL will need the approval of
e.g. jochen@ and those points will be reviewed in 
more depth.

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:100:
DCHECK(MediaStream::GetMediaStream(media_stream));
On 2016/01/07 09:18:00, perkj_chrome wrote:
> no need for this check. The same will happen on the line below if
> MediaStream::GetMediaStream return null.

Done.

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:209:
WriteData(base::StringPiece(""));
On 2016/01/07 09:18:00, perkj_chrome wrote:
> Why is WriteData(base::StringPiece necessary here but not in the normal stop
> condition? If it is required always, move to stop() ? 

Actually both writeData() and stop() are not needed
since client->onError() (new name of fail...() ) will do
all necessary steps to stop the recording. Both 
removed.

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream.cc:23: observers_.clear();
On 2016/01/07 09:18:00, perkj_chrome wrote:
> Why this change? Is there a case when the observers can outlive the
MediaStream?
Yes, MediaRecorder is destroyed independently from the MediaStream().

> It that case, how do they know that they can not call RemoveObserver ? 

Because this is the destructor of the class, that is accessed
via l.14
MediaStream* MediaStream::GetMediaStream(...)
once the dtor has been called, noone should try to
de-register as observer, right?

Correct me if I'm wrong, but this DCHECK was there to
debug some strange race condition appeared on garbage
collection between the only current observer, WebRtc,
and this MediaStream. But honestly, it does not make
sense that MediaStream waits for the Observers to
deregister before going down, specially taken into 
account the complete lack of guarantee from Blink on
the destruction order.

Note that I brought this up in the CL description.

https://codereview.chromium.org/1543673002/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.cc:796:
AddMediaStreamTowebMediaStream(stream);
So just to confirm, at this point I cannot just do
stream->setExtraData(new content::MediaStream());
because here I can only depend on content/public
and MediaStream is in content/renderer/media.

Actually it would be possible to move media_stream.{cc,h} 
to content/public/renderer, but I don't think is worth
for testing infrastructure, and would increase the
public API surface of content...

[mcasas, 2016-01-08 00:24:39]

Patchset #3 (id:40001) has been deleted

[perkj_chrome, 2016-01-08 07:54:50]

A question - why do you need the layout test changes if add a content browser
test ? 


layout tests are nice but the problem you hit with MockUserMediaClient does not
exist if you use a content browser test since they do not use the mock. 

Also the defect your reference is about stopping tracks that are recorded - not
adding or removing tracks. Will you fix that in a separate cl or is that already
fixed?

https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
File components/test_runner/mock_web_user_media_client.cc (right):

https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
components/test_runner/mock_web_user_media_client.cc:166:
delegate_->AddMediaStream(&stream);
On 2016/01/08 00:14:12, mcasas wrote:
> On 2016/01/07 09:18:00, perkj_chrome wrote:
> > this seems wrong. Why not just do stream.setExtraData(new
> > content::MediaStream()) if you need this.
> 
> The gist of this is that test_runner cannot depend on 
> content/ [1]. 
> 
> (Instead, the idea is to add stuff to a WebTestDelegate  
> and implement that accordingly).
> 
> 
> [1] https://codereview.chromium.org/1371373002/#msg37

I see. So MockWebUserMediaClient does not actually mock the real implementation
that well.
However, it feels wrong to add a public API in content/public/media_stream that
should never be called by anyone but the layout tests. It should never be called
since content::MediaStream  is created from  MediaStreamCenter if the
mediastream is created from JS.  If the stream has been created in
user_media_client, user_media_client creates content::MediaStream.

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream.cc (right):

https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream.cc:23: observers_.clear();
On 2016/01/08 00:14:12, mcasas wrote:
> On 2016/01/07 09:18:00, perkj_chrome wrote:
> > Why this change? Is there a case when the observers can outlive the
> MediaStream?
> Yes, MediaRecorder is destroyed independently from the MediaStream().
> 
> > It that case, how do they know that they can not call RemoveObserver ? 
> 
> Because this is the destructor of the class, that is accessed
> via l.14
> MediaStream* MediaStream::GetMediaStream(...)
> once the dtor has been called, noone should try to
> de-register as observer, right?

Right - and hence the DCHECK that no observer is still valid. Because that could
lead to a heap use after free if the observer call RemoveObserver after the
dtor.

A content::MediaStream will never be destroyed as long as the blink MediaStream
exist. And the blink MediaStream exists as long has there is a reference to it.

So the DCHECK(observers_.empty()) is correct I would say.

> 
> Correct me if I'm wrong, but this DCHECK was there to
> debug some strange race condition appeared on garbage
> collection between the only current observer, WebRtc,
> and this MediaStream. But honestly, it does not make
> sense that MediaStream waits for the Observers to
> deregister before going down, specially taken into 
> account the complete lack of guarantee from Blink on
> the destruction order.
> 
> Note that I brought this up in the CL description.

[mcasas, 2016-01-08 18:35:12]

On 2016/01/08 07:54:50, perkj_chrome wrote:
> A question - why do you need the layout test changes if add a content browser
> test ? 
> 
> 
> layout tests are nice but the problem you hit with MockUserMediaClient does
not
> exist if you use a content browser test since they do not use the mock. 
> 
> Also the defect your reference is about stopping tracks that are recorded -
not
> adding or removing tracks. Will you fix that in a separate cl or is that
already
> fixed?
> 
>
https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
> File components/test_runner/mock_web_user_media_client.cc (right):
> 
>
https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
> components/test_runner/mock_web_user_media_client.cc:166:
> delegate_->AddMediaStream(&stream);
> On 2016/01/08 00:14:12, mcasas wrote:
> > On 2016/01/07 09:18:00, perkj_chrome wrote:
> > > this seems wrong. Why not just do stream.setExtraData(new
> > > content::MediaStream()) if you need this.
> > 
> > The gist of this is that test_runner cannot depend on 
> > content/ [1]. 
> > 
> > (Instead, the idea is to add stuff to a WebTestDelegate  
> > and implement that accordingly).
> > 
> > 
> > [1] https://codereview.chromium.org/1371373002/#msg37
> 
> I see. So MockWebUserMediaClient does not actually mock the real
implementation
> that well.
> However, it feels wrong to add a public API in content/public/media_stream
that
> should never be called by anyone but the layout tests. It should never be
called
> since content::MediaStream  is created from  MediaStreamCenter if the
> mediastream is created from JS.  If the stream has been created in
> user_media_client, user_media_client creates content::MediaStream.
> 
>
https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
> File content/renderer/media/media_stream.cc (right):
> 
>
https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
> content/renderer/media/media_stream.cc:23: observers_.clear();
> On 2016/01/08 00:14:12, mcasas wrote:
> > On 2016/01/07 09:18:00, perkj_chrome wrote:
> > > Why this change? Is there a case when the observers can outlive the
> > MediaStream?
> > Yes, MediaRecorder is destroyed independently from the MediaStream().
> > 
> > > It that case, how do they know that they can not call RemoveObserver ? 
> > 
> > Because this is the destructor of the class, that is accessed
> > via l.14
> > MediaStream* MediaStream::GetMediaStream(...)
> > once the dtor has been called, noone should try to
> > de-register as observer, right?
> 
> Right - and hence the DCHECK that no observer is still valid. Because that
could
> lead to a heap use after free if the observer call RemoveObserver after the
> dtor.
> 
> A content::MediaStream will never be destroyed as long as the blink
MediaStream
> exist. And the blink MediaStream exists as long has there is a reference to
it.
> 
> So the DCHECK(observers_.empty()) is correct I would say.
> 
> > 
> > Correct me if I'm wrong, but this DCHECK was there to
> > debug some strange race condition appeared on garbage
> > collection between the only current observer, WebRtc,
> > and this MediaStream. But honestly, it does not make
> > sense that MediaStream waits for the Observers to
> > deregister before going down, specially taken into 
> > account the complete lack of guarantee from Blink on
> > the destruction order.
> > 
> > Note that I brought this up in the CL description.

I think we're agreeing to disagree, so I'll try to do this
observing purely inside Blink by making MediaRecorder
listen to WebMediaStream::on{add,remove}track instead
of doing it inside Cr. Thanks for your time reviewer(s)
and please hold up on this!

[mcasas, 2016-01-09 02:59:05]

On 2016/01/08 18:35:12, mcasas wrote:
> On 2016/01/08 07:54:50, perkj_chrome wrote:
> > A question - why do you need the layout test changes if add a content
browser
> > test ? 
> > 
> > 
> > layout tests are nice but the problem you hit with MockUserMediaClient does
> not
> > exist if you use a content browser test since they do not use the mock. 
> > 
> > Also the defect your reference is about stopping tracks that are recorded -
> not
> > adding or removing tracks. Will you fix that in a separate cl or is that
> already
> > fixed?
> > 
> >
>
https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
> > File components/test_runner/mock_web_user_media_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/1543673002/diff/1/components/test_runner/mock...
> > components/test_runner/mock_web_user_media_client.cc:166:
> > delegate_->AddMediaStream(&stream);
> > On 2016/01/08 00:14:12, mcasas wrote:
> > > On 2016/01/07 09:18:00, perkj_chrome wrote:
> > > > this seems wrong. Why not just do stream.setExtraData(new
> > > > content::MediaStream()) if you need this.
> > > 
> > > The gist of this is that test_runner cannot depend on 
> > > content/ [1]. 
> > > 
> > > (Instead, the idea is to add stuff to a WebTestDelegate  
> > > and implement that accordingly).
> > > 
> > > 
> > > [1] https://codereview.chromium.org/1371373002/#msg37
> > 
> > I see. So MockWebUserMediaClient does not actually mock the real
> implementation
> > that well.
> > However, it feels wrong to add a public API in content/public/media_stream
> that
> > should never be called by anyone but the layout tests. It should never be
> called
> > since content::MediaStream  is created from  MediaStreamCenter if the
> > mediastream is created from JS.  If the stream has been created in
> > user_media_client, user_media_client creates content::MediaStream.
> > 
> >
>
https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
> > File content/renderer/media/media_stream.cc (right):
> > 
> >
>
https://codereview.chromium.org/1543673002/diff/1/content/renderer/media/medi...
> > content/renderer/media/media_stream.cc:23: observers_.clear();
> > On 2016/01/08 00:14:12, mcasas wrote:
> > > On 2016/01/07 09:18:00, perkj_chrome wrote:
> > > > Why this change? Is there a case when the observers can outlive the
> > > MediaStream?
> > > Yes, MediaRecorder is destroyed independently from the MediaStream().
> > > 
> > > > It that case, how do they know that they can not call RemoveObserver ? 
> > > 
> > > Because this is the destructor of the class, that is accessed
> > > via l.14
> > > MediaStream* MediaStream::GetMediaStream(...)
> > > once the dtor has been called, noone should try to
> > > de-register as observer, right?
> > 
> > Right - and hence the DCHECK that no observer is still valid. Because that
> could
> > lead to a heap use after free if the observer call RemoveObserver after the
> > dtor.
> > 
> > A content::MediaStream will never be destroyed as long as the blink
> MediaStream
> > exist. And the blink MediaStream exists as long has there is a reference to
> it.
> > 
> > So the DCHECK(observers_.empty()) is correct I would say.
> > 
> > > 
> > > Correct me if I'm wrong, but this DCHECK was there to
> > > debug some strange race condition appeared on garbage
> > > collection between the only current observer, WebRtc,
> > > and this MediaStream. But honestly, it does not make
> > > sense that MediaStream waits for the Observers to
> > > deregister before going down, specially taken into 
> > > account the complete lack of guarantee from Blink on
> > > the destruction order.
> > > 
> > > Note that I brought this up in the CL description.
> 
> I think we're agreeing to disagree, so I'll try to do this
> observing purely inside Blink by making MediaRecorder
> listen to WebMediaStream::on{add,remove}track instead
> of doing it inside Cr. Thanks for your time reviewer(s)
> and please hold up on this!

And FTR the alternative Blink-only implementation is in
https://codereview.chromium.org/1570263002/. Closing this
CL, cheers!