Bug 125243 – [MediaStream] Firing negotiationneeded event upon track add/remove on MediaStream

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

Spec states that: In particular, if an RTCPeerConnection object is consuming a MediaStream on which a track is added, by, e.g., the addTrack() method being invoked, the RTCPeerConnection object must fire the "negotiationneeded" event. Removal of media components must also trigger "negotiationneeded".

[Thiago de Barros Lacerda]

Created attachment 218429 [details]
Patch

[Eric Carlson]

Comment on attachment 218429 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:158
> +            (*observer)->didAddRemoveTrack();

I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack".

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:170
> +    for (auto stream = m_localStreams.begin(); stream != m_localStreams.end(); ++stream)
> +        (*stream)->removeObserver(this);

The vector length won't change during the loop, so a more efficient way to do this would be to cache the end in the initialization:

for (auto stream = m_localStreams.begin(), end = m_localStreams.end(); stream != end; ++ stream)

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395
>      bool valid = m_peerHandler->addStream(stream->privateStream(), constraints);
>      if (!valid)
>          ec = SYNTAX_ERR;
> +    else {
> +        m_localStreams.append(stream);
> +        stream->addObserver(this);
> +    }

It would be good to have a test for this change.

> LayoutTests/ChangeLog:13
> +        * fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:

Don't we also need results for this test?

[Thiago de Barros Lacerda]

(In reply to comment #2)
> (From update of attachment 218429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218429&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:158
> > +            (*observer)->didAddRemoveTrack();
> 
> I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack".

Maybe two methods will be too much, I will rename it to didAddOrRemoveTrack. Is that OK to you?

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:170
> > +    for (auto stream = m_localStreams.begin(); stream != m_localStreams.end(); ++stream)
> > +        (*stream)->removeObserver(this);
> 
> The vector length won't change during the loop, so a more efficient way to do this would be to cache the end in the initialization:

Makes sense

> 
> for (auto stream = m_localStreams.begin(), end = m_localStreams.end(); stream != end; ++ stream)
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395
> >      bool valid = m_peerHandler->addStream(stream->privateStream(), constraints);
> >      if (!valid)
> >          ec = SYNTAX_ERR;
> > +    else {
> > +        m_localStreams.append(stream);
> > +        stream->addObserver(this);
> > +    }
> 
> It would be good to have a test for this change.

How would I test that? The observers are not accessible in javascript... and API test does that reaches it. But, in some way, the layout test that I've updated guarantees it, since negotiationneeded will only be fired if there are observers registered

> 
> > LayoutTests/ChangeLog:13
> > +        * fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:
> 
> Don't we also need results for this test?

My bad, I forgot

[Thiago de Barros Lacerda]

Created attachment 218510 [details]
Patch

[Eric Carlson]

Comment on attachment 218429 [details]
Patch

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

>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:158
>>> +            (*observer)->didAddRemoveTrack();
>> 
>> I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack".
> 
> Maybe two methods will be too much, I will rename it to didAddOrRemoveTrack. Is that OK to you?

That seems fine, thanks.

>>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395
>>> +    }
>> 
>> It would be good to have a test for this change.
> 
> How would I test that? The observers are not accessible in javascript... and API test does that reaches it. But, in some way, the layout test that I've updated guarantees it, since negotiationneeded will only be fired if there are observers registered

Can't you pass an invalid constraint when there is a 'negotiationneeded' event listener registered to ensure that the event is not fired?

[Eric Carlson]

Comment on attachment 218510 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:157
> +        for (auto observer = m_observers.begin(); observer != m_observers.end(); ++observer)

The length of this vector is also invariant: 

for (auto observer = m_observers.begin(), end = m_observers.end(); observer != end; ++observer)

> Source/WebCore/Modules/mediastream/MediaStream.cpp:192
> +            (*observer)->didAddOrRemoveTrack();

Ditto.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:388
> +    size_t pos = m_observers.find(observer);
> +    if (pos == notFound)

Nit: the local variable is unnecessary.

> LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:33
> +            function onNegotiationNeeded2(event) {

I see that you are just following the existing style of this test, but a function's opening brace should be on a new line.

> LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:38
> +            function onNegotiationNeeded3(event) {

Ditto.

> LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:53
> +            function removeTrack() {

Ditto.

> LayoutTests/fast/mediastream/RTCPeerConnection-onnegotiationneeded.html:61
> +            function addTrack() {

Ditto.

[Eric Carlson]

Comment on attachment 218510 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:191
> +        for (auto observer = m_observers.begin(); observer != m_observers.end(); ++observer)

"Ditto" should be here, not on the next line.

[Thiago de Barros Lacerda]

(In reply to comment #5)
> (From update of attachment 218429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218429&action=review
> 
> >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:158
> >>> +            (*observer)->didAddRemoveTrack();
> >> 
> >> I am not wild about the name "didAddRemoveTrack". I would prefer to see two methods, one for add and another for remove, or maybe name like "didAddOrRemoveTrack".
> > 
> > Maybe two methods will be too much, I will rename it to didAddOrRemoveTrack. Is that OK to you?
> 
> That seems fine, thanks.
> 
> >>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:395
> >>> +    }
> >> 
> >> It would be good to have a test for this change.
> > 
> > How would I test that? The observers are not accessible in javascript... and API test does that reaches it. But, in some way, the layout test that I've updated guarantees it, since negotiationneeded will only be fired if there are observers registered
> 
> Can't you pass an invalid constraint when there is a 'negotiationneeded' event listener registered to ensure that the event is not fired?

Sorry, I thought you were talking about the add/removeObserver method, I was looking at the wrong place. Yes, this can be tested, I'm updating a test to handle that

[Thiago de Barros Lacerda]

Created attachment 218522 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 218522 [details]
Patch

Clearing flags on attachment: 218522

Committed r160181: <http://trac.webkit.org/changeset/160181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.