Bug 121954 – [MediaStream API] allow a stream source to be shared

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

[Eric Carlson]

The MediaStream API allows a stream source to be rendered to multiple outputs, eg. by using MediaTrack.clone() and/or MediaStream.addTrack(), but we currently assume that each source is owned by a single track.

[Thiago de Barros Lacerda]

Created attachment 214107 [details]
First draft

[Thiago de Barros Lacerda]

I think this will be a long review. I'm proposing this first draft so reviewers can give feedback

[Eric Carlson]

Comment on attachment 214107 [details]
First draft

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

This is a great start, but I think it definitely needs a test to make sure the states are being set correctly. For example, what makes a source->readyState() go to "Ended"?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:197
> +    m_descriptor->addSource(prpTrack->source());

You should use "track" rather than "prpTrack".

> Source/WebCore/Modules/mediastream/MediaStream.cpp:243
> +bool MediaStream::hasTrackWithSource(PassRefPtr<MediaStreamSource> source)

Nit: I think "haveTrackWithSource" would be a slightly better name.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:257
> +    if (source->type() == MediaStreamSource::Audio) {
> +        for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) {
> +            if (track->source() == source.get())
> +                return true;
> +        }
> +    } else {
> +        for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) {
> +            if (track->source() == source.get())
> +                return true;
> +        }
> +    }
> +
> +    return false;

The "else" isn't necessary:

if (source->type() == MediaStreamSource::Audio) {
    for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) {
        if (track->source() == source.get())
            return true;
    }
    return false;
}

for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) {
    if (track->source() == source.get())
        return true;
}

return false;

> Source/WebCore/Modules/mediastream/MediaStream.cpp:346
>              index = i;
> +            removedTracks.append((*tracks)[i]);
> +            tracks->remove(i);
>              break;

Don't you want to look through all of the tracks instead of breaking on the first one that uses the source?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:353
> +    if (index == notFound)
> +        return;

if (!removedTracks.size()) return;

[Thiago de Barros Lacerda]

(In reply to comment #3)
> (From update of attachment 214107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214107&action=review
> 
> This is a great start, but I think it definitely needs a test to make sure the states are being set correctly. For example, what makes a source->readyState() go to "Ended"?

Yep, it needs tests. I just uploaded it as a first draft, without tests for you guys to take a look at my approach on the problem :)

I think that only the backend the provided the source can tell if a source has ended or not. Then the MediaStreamSource is going to tell the others that are interested in its state (by the observers) that it has ended, for instance.
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:197
> > +    m_descriptor->addSource(prpTrack->source());
> 
> You should use "track" rather than "prpTrack".

My bad.
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:243
> > +bool MediaStream::hasTrackWithSource(PassRefPtr<MediaStreamSource> source)
> 
> Nit: I think "haveTrackWithSource" would be a slightly better name.

I agree
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:257
> > +    if (source->type() == MediaStreamSource::Audio) {
> > +        for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) {
> > +            if (track->source() == source.get())
> > +                return true;
> > +        }
> > +    } else {
> > +        for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) {
> > +            if (track->source() == source.get())
> > +                return true;
> > +        }
> > +    }
> > +
> > +    return false;
> 
> The "else" isn't necessary:

good point
> 
> if (source->type() == MediaStreamSource::Audio) {
>     for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) {
>         if (track->source() == source.get())
>             return true;
>     }
>     return false;
> }
> 
> for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) {
>     if (track->source() == source.get())
>         return true;
> }
> 
> return false;
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:346
> >              index = i;
> > +            removedTracks.append((*tracks)[i]);
> > +            tracks->remove(i);
> >              break;
> 
> Don't you want to look through all of the tracks instead of breaking on the first one that uses the source?

Silly me
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:353
> > +    if (index == notFound)
> > +        return;
> 
> if (!removedTracks.size()) return;

[Thiago de Barros Lacerda]

Created attachment 214121 [details]
Fixing last patch (no tests yet)

[Eric Carlson]

Comment on attachment 214121 [details]
Fixing last patch (no tests yet)

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

r=me but you need to fix the ChangeLog comment. Please consider the other suggestions.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

This won't commit with the "OOPS!" in place. I would change it to something like "No new tests, no change in functionality".

> Source/WebCore/ChangeLog:32
> +        * Modules/mediastream/MediaStream.cpp:
> +        (WebCore::MediaStream::MediaStream):
> +        (WebCore::MediaStream::addTrack):
> +        (WebCore::MediaStream::removeTrack):
> +        (WebCore::MediaStream::haveTrackWithSource):
> +        (WebCore::MediaStream::addRemoteSource):
> +        (WebCore::MediaStream::removeRemoteSource):
> +        * Modules/mediastream/MediaStream.h:
> +        * Modules/mediastream/MediaStreamTrack.cpp:
> +        (WebCore::MediaStreamTrack::addObserver):
> +        (WebCore::MediaStreamTrack::removeObserver):
> +        (WebCore::MediaStreamTrack::trackDidEnd):
> +        * Modules/mediastream/MediaStreamTrack.h:
> +        * platform/mediastream/MediaStreamDescriptor.cpp:
> +        (WebCore::MediaStreamDescriptor::removeSource):
> +        (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> +        (WebCore::MediaStreamDescriptor::setEnded):
> +        * platform/mediastream/MediaStreamDescriptor.h:
> +        (WebCore::MediaStreamDescriptor::~MediaStreamDescriptor):
> +        * platform/mediastream/MediaStreamSource.cpp:
> +        (WebCore::MediaStreamSource::MediaStreamSource):
> +        (WebCore::MediaStreamSource::reset):
> +        * platform/mediastream/MediaStreamSource.h:

It would be nice to have brief descriptions of what changed in each method. I think this can really help later when investigating past changes.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:127
> +        m_audioTracks.append(track);

Nit: m_audioTracks.append(track.release());

> Source/WebCore/Modules/mediastream/MediaStream.cpp:135
> +        m_videoTracks.append(track);

Ditto

> Source/WebCore/Modules/mediastream/MediaStream.cpp:342
>          if ((*tracks)[i]->source() == source) {

Nit: you might as well update this loop to use the new hotness - for (const RefPtr<MediaStreamTrack>& track : *tracks) ...

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:106
> +    for (size_t i = 0; i < audioSources.size(); i++)

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109
> +    for (size_t i = 0; i < videoSources.size(); i++)

Ditto.

[Thiago de Barros Lacerda]

(In reply to comment #6)
> (From update of attachment 214121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214121&action=review
> 
> r=me but you need to fix the ChangeLog comment. Please consider the other suggestions.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> This won't commit with the "OOPS!" in place. I would change it to something like "No new tests, no change in functionality".
> 
> > Source/WebCore/ChangeLog:32
> > +        * Modules/mediastream/MediaStream.cpp:
> > +        (WebCore::MediaStream::MediaStream):
> > +        (WebCore::MediaStream::addTrack):
> > +        (WebCore::MediaStream::removeTrack):
> > +        (WebCore::MediaStream::haveTrackWithSource):
> > +        (WebCore::MediaStream::addRemoteSource):
> > +        (WebCore::MediaStream::removeRemoteSource):
> > +        * Modules/mediastream/MediaStream.h:
> > +        * Modules/mediastream/MediaStreamTrack.cpp:
> > +        (WebCore::MediaStreamTrack::addObserver):
> > +        (WebCore::MediaStreamTrack::removeObserver):
> > +        (WebCore::MediaStreamTrack::trackDidEnd):
> > +        * Modules/mediastream/MediaStreamTrack.h:
> > +        * platform/mediastream/MediaStreamDescriptor.cpp:
> > +        (WebCore::MediaStreamDescriptor::removeSource):
> > +        (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> > +        (WebCore::MediaStreamDescriptor::setEnded):
> > +        * platform/mediastream/MediaStreamDescriptor.h:
> > +        (WebCore::MediaStreamDescriptor::~MediaStreamDescriptor):
> > +        * platform/mediastream/MediaStreamSource.cpp:
> > +        (WebCore::MediaStreamSource::MediaStreamSource):
> > +        (WebCore::MediaStreamSource::reset):
> > +        * platform/mediastream/MediaStreamSource.h:
> 
> It would be nice to have brief descriptions of what changed in each method. I think this can really help later when investigating past changes.
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:127
> > +        m_audioTracks.append(track);
> 
> Nit: m_audioTracks.append(track.release());

OK

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:135
> > +        m_videoTracks.append(track);
> 
> Ditto

OK

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:342
> >          if ((*tracks)[i]->source() == source) {
> 
> Nit: you might as well update this loop to use the new hotness - for (const RefPtr<MediaStreamTrack>& track : *tracks) ...

But I need the indexes to remove the tracks from the track vector :(.
Also, by looking carefully at this loop, I'm removing the element while iterating on the vector, this is not very safe. So I will keep the for (with no range based loop) and store the indexes to be removed in another vector and remove them in the loop below it (which is dispatching the events). Is that ok for you?

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:106
> > +    for (size_t i = 0; i < audioSources.size(); i++)
> 
> Ditto.

OK

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109
> > +    for (size_t i = 0; i < videoSources.size(); i++)
> 
> Ditto.

OK

[Thiago de Barros Lacerda]

Created attachment 215099 [details]
Patch

[Eric Carlson]

Comment on attachment 215099 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:356
> +    for (int i : tracksToRemove) {
> +        RefPtr<MediaStreamTrack> track = (*tracks)[i];
> +        track->removeObserver(this);
> +        tracks->remove(i);
> +        scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track.release()));
> +    }

Doesn't this need to loop backwards through the tracks since you are removing them by index?

[Darin Adler]

Comment on attachment 215099 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:349
> +    if (tracksToRemove.isEmpty())
> +        return;

This early return doesn’t do anything useful.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:351
> +    for (int i : tracksToRemove) {

Not all the compilers we support work with this syntax.

[Thiago de Barros Lacerda]

(In reply to comment #10)
> (From update of attachment 215099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215099&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:349
> > +    if (tracksToRemove.isEmpty())
> > +        return;
> 
> This early return doesn’t do anything useful.

you are right.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:351
> > +    for (int i : tracksToRemove) {
> 
> Not all the compilers we support work with this syntax.

There are other parts in WebKit code that are already using range based for loops

[Thiago de Barros Lacerda]

Created attachment 215105 [details]
Patch with requested changeds

[Thiago de Barros Lacerda]

Created attachment 215107 [details]
Patch with requested changes

[Thiago de Barros Lacerda]

Created attachment 215108 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215108 [details]
Patch

Clearing flags on attachment: 215108

Committed r157958: <http://trac.webkit.org/changeset/157958>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.