Bug 123477 – Adding addRemoteTrack and removeRemoteTrack functions to MediaStreamDescriptor and MediaStream

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Thiago de Barros Lacerda]

When the backend report that a remote track has been added or removed by a remote peer, we must be able to add it to its MediaStream

[Thiago de Barros Lacerda]

Created attachment 215441 [details]
Patch

[Eric Carlson]

Comment on attachment 215441 [details]
Patch

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

As we talked about on irc, it would be good to get rid of MediaStreamDescriptor::addSource and MediaStreamDescriptor::removeSource.

> Source/WebCore/ChangeLog:27
> +        * Modules/mediastream/MediaStream.cpp:
> +        (WebCore::MediaStream::addTrack):
> +        (WebCore::MediaStream::removeTrack):
> +        (WebCore::MediaStream::addRemoteSource):
> +        (WebCore::MediaStream::removeRemoteSource):
> +        (WebCore::MediaStream::addRemoteTrack):
> +        (WebCore::MediaStream::removeRemoteTrack):
> +        (WebCore::MediaStream::getTrackVector):
> +        * Modules/mediastream/MediaStream.h:
> +        * Modules/mediastream/MediaStreamTrack.h:
> +        (WebCore::MediaStreamTrack::privateTrack):
> +        * platform/mediastream/MediaStreamDescriptor.cpp:
> +        (WebCore::MediaStreamDescriptor::addRemoteTrack):
> +        (WebCore::MediaStreamDescriptor::removeRemoteTrack):
> +        * platform/mediastream/MediaStreamDescriptor.h:

Comments please.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:163
> +    MediaStreamTrackVector* tracks = getTrackVector(track->source()->type());

This won't work because "track" is NULL after begin used above. You need to add something like "RefPtr<MediaStreamTrack> track = prpTrack" before the PassRefPtr is dereferenced.

Nit: maybe change the name to "getTrackVectorForType"?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:196
> +    MediaStreamTrackVector* tracks = getTrackVector(track->source()->type());
> +
> +    if (!tracks)
> +        return false;
>  
> +    size_t pos = tracks->find(track);

Ditto about using a PassRefPtr.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:279
> +    RefPtr<MediaStreamTrackPrivate> track = MediaStreamTrackPrivate::create(source);
> +    addRemoteTrack(track.get());

Nit: this can be done on a single line.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:342
>          scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track.release()));

Why don't we dispatch the event in removeTrack?

> Source/WebCore/Modules/mediastream/MediaStream.h:101
>      virtual void addRemoteSource(MediaStreamSource*) OVERRIDE FINAL;
>      virtual void removeRemoteSource(MediaStreamSource*) OVERRIDE FINAL;
> +    virtual void addRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;
> +    virtual void removeRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;

Why do we need both versions, can we just have add/removeRemoteTrack?

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:53
>      virtual void addRemoteSource(MediaStreamSource*) = 0;
>      virtual void removeRemoteSource(MediaStreamSource*) = 0;

Can these be removed - is there ever a reason to pass the source directly to MediaStream instead of creating the private track and passing that?

[Thiago de Barros Lacerda]

Comment on attachment 215441 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:163
>> +    MediaStreamTrackVector* tracks = getTrackVector(track->source()->type());
> 
> This won't work because "track" is NULL after begin used above. You need to add something like "RefPtr<MediaStreamTrack> track = prpTrack" before the PassRefPtr is dereferenced.
> 
> Nit: maybe change the name to "getTrackVectorForType"?

But here I'm passing the Enum type to the getTrackVector, not the track itself, will it really have deref called?

getTrackVectorForType sounds better.

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:196
>> +    size_t pos = tracks->find(track);
> 
> Ditto about using a PassRefPtr.

OK, here I agree that a deref will happen.

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:279
>> +    addRemoteTrack(track.get());
> 
> Nit: this can be done on a single line.

OK.

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:342
>>          scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track.release()));
> 
> Why don't we dispatch the event in removeTrack?

Because removeTrack (receiving only a track) is used by both removeRemoteTrack and removeTrack (called by JS, which is not supposed to fire the event).

>> Source/WebCore/Modules/mediastream/MediaStream.h:101
>> +    virtual void removeRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;
> 
> Why do we need both versions, can we just have add/removeRemoteTrack?

What do you mean with both versions?

>> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:53
>>      virtual void removeRemoteSource(MediaStreamSource*) = 0;
> 
> Can these be removed - is there ever a reason to pass the source directly to MediaStream instead of creating the private track and passing that?

Yes, as we have decided on irc, they will be removed. But removing them will affect more files and will increase the size of this patch, getting out of its scope and making the review more difficult.
I can do it in a separate patch. Is that OK for you?

[Eric Carlson]

Comment on attachment 215441 [details]
Patch

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

>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:163
>>> +    MediaStreamTrackVector* tracks = getTrackVector(track->source()->type());
>> 
>> This won't work because "track" is NULL after begin used above. You need to add something like "RefPtr<MediaStreamTrack> track = prpTrack" before the PassRefPtr is dereferenced.
>> 
>> Nit: maybe change the name to "getTrackVectorForType"?
> 
> But here I'm passing the Enum type to the getTrackVector, not the track itself, will it really have deref called?
> 
> getTrackVectorForType sounds better.

The patch may be misleading because it doesn't apply, but it looks like this function has both "getTrackById(track->id())" and "getTrackVector(track->source()->type())".

>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:342
>>>          scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track.release()));
>> 
>> Why don't we dispatch the event in removeTrack?
> 
> Because removeTrack (receiving only a track) is used by both removeRemoteTrack and removeTrack (called by JS, which is not supposed to fire the event).

OK. A comment explaining that (maybe in removeTrack?) would be useful.

>>> Source/WebCore/Modules/mediastream/MediaStream.h:101
>>> +    virtual void removeRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;
>> 
>> Why do we need both versions, can we just have add/removeRemoteTrack?
> 
> What do you mean with both versions?

addRemoteSource and addRemoteTrack, removeRemoteSource and removeRemoteTrack.

>>> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:53
>>>      virtual void removeRemoteSource(MediaStreamSource*) = 0;
>> 
>> Can these be removed - is there ever a reason to pass the source directly to MediaStream instead of creating the private track and passing that?
> 
> Yes, as we have decided on irc, they will be removed. But removing them will affect more files and will increase the size of this patch, getting out of its scope and making the review more difficult.
> I can do it in a separate patch. Is that OK for you?

That sounds fine.

[Thiago de Barros Lacerda]

(In reply to comment #4)
> (From update of attachment 215441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215441&action=review
> 
> >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:163
> >>> +    MediaStreamTrackVector* tracks = getTrackVector(track->source()->type());
> >> 
> >> This won't work because "track" is NULL after begin used above. You need to add something like "RefPtr<MediaStreamTrack> track = prpTrack" before the PassRefPtr is dereferenced.
> >> 
> >> Nit: maybe change the name to "getTrackVectorForType"?
> > 
> > But here I'm passing the Enum type to the getTrackVector, not the track itself, will it really have deref called?
> > 
> > getTrackVectorForType sounds better.
> 
> The patch may be misleading because it doesn't apply, but it looks like this function has both "getTrackById(track->id())" and "getTrackVector(track->source()->type())".

The patch was based in https://bugs.webkit.org/show_bug.cgi?id=123443, which changed a lot. I will update this one and push the updated patch.

> 
> >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:342
> >>>          scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track.release()));
> >> 
> >> Why don't we dispatch the event in removeTrack?
> > 
> > Because removeTrack (receiving only a track) is used by both removeRemoteTrack and removeTrack (called by JS, which is not supposed to fire the event).
> 
> OK. A comment explaining that (maybe in removeTrack?) would be useful.
> 
> >>> Source/WebCore/Modules/mediastream/MediaStream.h:101
> >>> +    virtual void removeRemoteTrack(MediaStreamTrackPrivate*) OVERRIDE FINAL;
> >> 
> >> Why do we need both versions, can we just have add/removeRemoteTrack?
> > 
> > What do you mean with both versions?
> 
> addRemoteSource and addRemoteTrack, removeRemoteSource and removeRemoteTrack.

The removal of those will be part of the other patch to remove add/remove/Source and remote source.
> 
> >>> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:53
> >>>      virtual void removeRemoteSource(MediaStreamSource*) = 0;
> >> 
> >> Can these be removed - is there ever a reason to pass the source directly to MediaStream instead of creating the private track and passing that?
> > 
> > Yes, as we have decided on irc, they will be removed. But removing them will affect more files and will increase the size of this patch, getting out of its scope and making the review more difficult.
> > I can do it in a separate patch. Is that OK for you?
> 
> That sounds fine.

[Thiago de Barros Lacerda]

Created attachment 215681 [details]
Updated patch

[Eric Carlson]

Comment on attachment 215681 [details]
Updated patch

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

> Source/WebCore/ChangeLog:19
> +        (WebCore::MediaStream::removeTrack): Splitted in two parts that can be used by old removeTrack and new
> +        removeRemoteTrack.
> +
> +        (WebCore::MediaStream::addRemoteSource): Reusing code in new addTrack method.

Nit: the blank lines between methods in the same file are not necessary.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:196
> +bool MediaStream::removeTrack(PassRefPtr<MediaStreamTrack> track)
> +{
> +    // This is a common part used by removeTrack called by JavaScript
> +    // and removeRemoteTrack and only removeRemoteTrack must fire removetrack event
> +    RefPtr<MediaStreamTrack> trackRef = track;

Nit: the convention is to prefix the PassRefPtr name with prp: "Unless the use of the argument is very simple, the argument should be transferred to a RefPtr at the start of the function; the argument can be named with a “prp” prefix in such cases."

http://www.webkit.org/coding/RefPtr.html

> Source/WebCore/Modules/mediastream/MediaStream.cpp:197
> +    MediaStreamTrackVector* tracks = getTrackVectorForType(trackRef->source()->type());

Nit: track->type() returns the same thing as track->source()->type()

> Source/WebCore/Modules/mediastream/MediaStream.cpp:302
> +    Vector<int> tracksToRemove;
> +    for (size_t i = 0; i < tracks->size(); ++i) {
> +        if ((*tracks)[i]->source() == source)
> +            tracksToRemove.append(i);
>      }

Nit: I know this isn't new code, but you could clean this up by using Vector<RefPtr<MediaStreamTrack>> and "for (auto ..." here and you wouldn't have to loop backwards below.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:322
> +    switch (privateTrack->source()->type()) {

Nit: privateTrack->type()

[Thiago de Barros Lacerda]

(In reply to comment #7)
> (From update of attachment 215681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215681&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +        (WebCore::MediaStream::removeTrack): Splitted in two parts that can be used by old removeTrack and new
> > +        removeRemoteTrack.
> > +
> > +        (WebCore::MediaStream::addRemoteSource): Reusing code in new addTrack method.
> 
> Nit: the blank lines between methods in the same file are not necessary.

Got it.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:196
> > +bool MediaStream::removeTrack(PassRefPtr<MediaStreamTrack> track)
> > +{
> > +    // This is a common part used by removeTrack called by JavaScript
> > +    // and removeRemoteTrack and only removeRemoteTrack must fire removetrack event
> > +    RefPtr<MediaStreamTrack> trackRef = track;
> 
> Nit: the convention is to prefix the PassRefPtr name with prp: "Unless the use of the argument is very simple, the argument should be transferred to a RefPtr at the start of the function; the argument can be named with a “prp” prefix in such cases."
> 
> http://www.webkit.org/coding/RefPtr.html

OK, changing that.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:197
> > +    MediaStreamTrackVector* tracks = getTrackVectorForType(trackRef->source()->type());
> 
> Nit: track->type() returns the same thing as track->source()->type()

Here I'm dealing with MediaStreamTrack, and it does not have type() method. Maybe you are confusing it with MediaStreamTrackPrivate?

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:302
> > +    Vector<int> tracksToRemove;
> > +    for (size_t i = 0; i < tracks->size(); ++i) {
> > +        if ((*tracks)[i]->source() == source)
> > +            tracksToRemove.append(i);
> >      }
> 
> Nit: I know this isn't new code, but you could clean this up by using Vector<RefPtr<MediaStreamTrack>> and "for (auto ..." here and you wouldn't have to loop backwards below.

I need the indexes to remove the track from the vector. But in fact I can reduce this code to only one loop, by removing the tracks while iterating the vector backwards.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:322
> > +    switch (privateTrack->source()->type()) {
> 
> Nit: privateTrack->type()

OK.

[Eric Carlson]

Comment on attachment 215681 [details]
Updated patch

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

>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:197
>>> +    MediaStreamTrackVector* tracks = getTrackVectorForType(trackRef->source()->type());
>> 
>> Nit: track->type() returns the same thing as track->source()->type()
> 
> Here I'm dealing with MediaStreamTrack, and it does not have type() method. Maybe you are confusing it with MediaStreamTrackPrivate?

Oops :-O.

>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:302
>>>      }
>> 
>> Nit: I know this isn't new code, but you could clean this up by using Vector<RefPtr<MediaStreamTrack>> and "for (auto ..." here and you wouldn't have to loop backwards below.
> 
> I need the indexes to remove the track from the vector. But in fact I can reduce this code to only one loop, by removing the tracks while iterating the vector backwards.

OK

[Thiago de Barros Lacerda]

Created attachment 215734 [details]
Updated patch

[WebKit Commit Bot]

Comment on attachment 215734 [details]
Updated patch

Clearing flags on attachment: 215734

Committed r158438: <http://trac.webkit.org/changeset/158438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.