Bug 123443 – Simplifying MediaStream and MediStreamDescriptor creation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Thiago de Barros Lacerda]

The internal process of creating a MediaStream and MediaStreamDescriptor was quite confusing and spread. We can take advantage of the platform implementation of MediaStreamTrack (aka MediaStreamTrackPrivate) and simplify the whole process.
A new constructor that receives vectors of MediaStreamTrackPrivate objects were added, then the check if a source already exists or if the tracks are all ended are now made in MediaStreamDescriptor.

[Thiago de Barros Lacerda]

Created attachment 215383 [details]
Patch

[Thiago de Barros Lacerda]

Created attachment 215419 [details]
Fixing old patch

[Eric Carlson]

Comment on attachment 215419 [details]
Fixing old patch

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

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:134
> +    unsigned providedTracksSize = audioPrivateTracks.size() + videoPrivateTracks.size();
> +    unsigned tracksSize = m_audioPrivateTracks.size() + m_videoPrivateTracks.size();
> +    // If tracks were provided and no one was added to the MediaStreamDescriptor's tracks, this means
> +    // that the tracks were all ended
> +    if (providedTracksSize > 0 && !tracksSize)
> +        m_ended = true;

This can be simplified by adding an early return when "!m_audioPrivateTracks.size() && !m_videoPrivateTracks.size()"

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:137
> +void MediaStreamDescriptor::addTrackAndSource(PassRefPtr<MediaStreamSource> source, PassRefPtr<MediaStreamTrackPrivate> track)

It looks like source is always the same as track->source(), so you can just pass the track.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:95
> +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;

I don't think this typedef is helpful, it requires someone not familiar with the code to search out the definition to figure out what it means.

[Thiago de Barros Lacerda]

(In reply to comment #3)
> (From update of attachment 215419 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215419&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:134
> > +    unsigned providedTracksSize = audioPrivateTracks.size() + videoPrivateTracks.size();
> > +    unsigned tracksSize = m_audioPrivateTracks.size() + m_videoPrivateTracks.size();
> > +    // If tracks were provided and no one was added to the MediaStreamDescriptor's tracks, this means
> > +    // that the tracks were all ended
> > +    if (providedTracksSize > 0 && !tracksSize)
> > +        m_ended = true;
> 
> This can be simplified by adding an early return when "!m_audioPrivateTracks.size() && !m_videoPrivateTracks.size()"

Makes sense.

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:137
> > +void MediaStreamDescriptor::addTrackAndSource(PassRefPtr<MediaStreamSource> source, PassRefPtr<MediaStreamTrackPrivate> track)
> 
> It looks like source is always the same as track->source(), so you can just pass the track.

Good catch :)

> 
> > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:95
> > +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;
> 
> I don't think this typedef is helpful, it requires someone not familiar with the code to search out the definition to figure out what it means.

I just followed the same approach as in MediaStreamSource, which defines MediaStreamSourceVector.

[Thiago de Barros Lacerda]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 215419 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=215419&action=review
> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:134
> > > +    unsigned providedTracksSize = audioPrivateTracks.size() + videoPrivateTracks.size();
> > > +    unsigned tracksSize = m_audioPrivateTracks.size() + m_videoPrivateTracks.size();
> > > +    // If tracks were provided and no one was added to the MediaStreamDescriptor's tracks, this means
> > > +    // that the tracks were all ended
> > > +    if (providedTracksSize > 0 && !tracksSize)
> > > +        m_ended = true;
> > 
> > This can be simplified by adding an early return when "!m_audioPrivateTracks.size() && !m_videoPrivateTracks.size()"
> 
> Makes sense.

No, we can't do that because there can be two reasons why privatetracks are empty: MediaStream was created with no tracks or sources or all tracks were ended.
So, early returning may miss the second and do not set the m_ended attribute

> 
> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:137
> > > +void MediaStreamDescriptor::addTrackAndSource(PassRefPtr<MediaStreamSource> source, PassRefPtr<MediaStreamTrackPrivate> track)
> > 
> > It looks like source is always the same as track->source(), so you can just pass the track.
> 
> Good catch :)
> 
> > 
> > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:95
> > > +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;
> > 
> > I don't think this typedef is helpful, it requires someone not familiar with the code to search out the definition to figure out what it means.
> 
> I just followed the same approach as in MediaStreamSource, which defines MediaStreamSourceVector.

[Thiago de Barros Lacerda]

Created attachment 215437 [details]
Updated patch

[Eric Carlson]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:95
> > > +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;
> > 
> > I don't think this typedef is helpful, it requires someone not familiar with the code to search out the definition to figure out what it means.
> 
> I just followed the same approach as in MediaStreamSource, which defines MediaStreamSourceVector.

We inherited that from the initial implementation, but that isn't necessarily a reason to keep it. 

Defining a typedef for a complex type can be useful, eg. it can help someone who doesn't know the code understand what it is used for, but I don't thinks "MediaStreamTrackPrivateVector" is any easier to understand than "Vector<RefPtr<MediaStreamTrackPrivate>>", and I think it actually makes it slightly harder to understand the code because you have to hunt down the typedef.

[Eric Carlson]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:95
> > > > +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;
> > > 
> > > I don't think this typedef is helpful, it requires someone not familiar with the code to search out the definition to figure out what it means.
> > 
> > I just followed the same approach as in MediaStreamSource, which defines MediaStreamSourceVector.
> 
> We inherited that from the initial implementation, but that isn't necessarily a reason to keep it. 
> 
> Defining a typedef for a complex type can be useful, eg. it can help someone who doesn't know the code understand what it is used for, but I don't thinks "MediaStreamTrackPrivateVector" is any easier to understand than "Vector<RefPtr<MediaStreamTrackPrivate>>", and I think it actually makes it slightly harder to understand the code because you have to hunt down the typedef.

FWIW, I think we should remove MediaStreamSourceVector and its cousins.

[Eric Carlson]

Comment on attachment 215437 [details]
Updated patch

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

This is close, but I think it is worth one more round.

> Source/WebCore/ChangeLog:34
> +        * Modules/mediastream/MediaStream.cpp:
> +        (WebCore::MediaStream::create):
> +        * Modules/webaudio/MediaStreamAudioDestinationNode.cpp:
> +        (WebCore::MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode):
> +        * platform/mediastream/MediaStreamDescriptor.cpp:
> +        (WebCore::MediaStreamDescriptor::create):
> +        (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> +        (WebCore::MediaStreamDescriptor::addTrackAndSource):
> +        (WebCore::MediaStreamDescriptor::haveSourceWithId):
> +        (WebCore::MediaStreamDescriptor::addTrack):
> +        (WebCore::MediaStreamDescriptor::removeTrack):
> +        * platform/mediastream/MediaStreamDescriptor.h:
> +        (WebCore::MediaStreamDescriptor::numberOfAudioTracks):
> +        (WebCore::MediaStreamDescriptor::audioTracks):
> +        (WebCore::MediaStreamDescriptor::numberOfVideoTracks):
> +        (WebCore::MediaStreamDescriptor::videoTracks):
> +        * platform/mediastream/MediaStreamTrackPrivate.h:
> +        * platform/mock/MockMediaStreamCenter.cpp:
> +        (WebCore::MockMediaStreamCenter::createMediaStream):

Nit: comments please.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:57
> +        audioTracks.append(MediaStreamTrackPrivate::create(stream->m_audioTracks[i]->source()));

Why are you adding the track's *source* to the array of *tracks*? Does this even compile?

> Source/WebCore/Modules/mediastream/MediaStream.cpp:60
> +        videoTracks.append(MediaStreamTrackPrivate::create(stream->m_videoTracks[i]->source()));

Ditto.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:77
> +    return MediaStream::create(context, MediaStreamDescriptor::create(audioTracks, videoTracks));

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:45
> +PassRefPtr<MediaStreamDescriptor> MediaStreamDescriptor::create(const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources)

Nit: You can return RefPtr here. PassRefPtrs aren't needed since Anders added move-semantics to the RefPtr class.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:50
> +PassRefPtr<MediaStreamDescriptor> MediaStreamDescriptor::create(const MediaStreamTrackPrivateVector& audioPrivateTracks, const MediaStreamTrackPrivateVector& videoPrivateTracks)

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:121
> +    for (size_t i = 0; i < audioSources.size(); i++)
> +        addTrackAndSource(MediaStreamTrackPrivate::create(audioSources[i]));
> +
> +    for (size_t i = 0; i < videoSources.size(); i++)
> +        addTrackAndSource(MediaStreamTrackPrivate::create(videoSources[i]));
> +}

Does m_ended need to be set if all sources have ended (readyState == Ended)?

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:136
> +    unsigned providedTracksSize = audioPrivateTracks.size() + videoPrivateTracks.size();
> +    unsigned tracksSize = m_audioPrivateTracks.size() + m_videoPrivateTracks.size();

I don't think these local variables are necessary.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:163
> +    for (size_t i = 0; i < sourceVector.size(); ++i) {
> +        if (id == sourceVector[i]->id())
> +            return true;
>      }
> +
> +    return false;

Can you use sourceVector.find() here?

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:59
> +    static PassRefPtr<MediaStreamDescriptor> create(const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources);
> +    static PassRefPtr<MediaStreamDescriptor> create(const MediaStreamTrackPrivateVector& audioPrivateTracks, const MediaStreamTrackPrivateVector& videoPrivateTracks);

Nit: You can return RefPtr here. PassRefPtrs aren't needed since Anders added move-semantics to the RefPtr class.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:119
> +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;

This is unnecessary.

[Thiago de Barros Lacerda]

(In reply to comment #9)
> (From update of attachment 215437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215437&action=review
> 
> This is close, but I think it is worth one more round.
> 
> > Source/WebCore/ChangeLog:34
> > +        * Modules/mediastream/MediaStream.cpp:
> > +        (WebCore::MediaStream::create):
> > +        * Modules/webaudio/MediaStreamAudioDestinationNode.cpp:
> > +        (WebCore::MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode):
> > +        * platform/mediastream/MediaStreamDescriptor.cpp:
> > +        (WebCore::MediaStreamDescriptor::create):
> > +        (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> > +        (WebCore::MediaStreamDescriptor::addTrackAndSource):
> > +        (WebCore::MediaStreamDescriptor::haveSourceWithId):
> > +        (WebCore::MediaStreamDescriptor::addTrack):
> > +        (WebCore::MediaStreamDescriptor::removeTrack):
> > +        * platform/mediastream/MediaStreamDescriptor.h:
> > +        (WebCore::MediaStreamDescriptor::numberOfAudioTracks):
> > +        (WebCore::MediaStreamDescriptor::audioTracks):
> > +        (WebCore::MediaStreamDescriptor::numberOfVideoTracks):
> > +        (WebCore::MediaStreamDescriptor::videoTracks):
> > +        * platform/mediastream/MediaStreamTrackPrivate.h:
> > +        * platform/mock/MockMediaStreamCenter.cpp:
> > +        (WebCore::MockMediaStreamCenter::createMediaStream):
> 
> Nit: comments please.

OK.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:57
> > +        audioTracks.append(MediaStreamTrackPrivate::create(stream->m_audioTracks[i]->source()));
> 
> Why are you adding the track's *source* to the array of *tracks*? Does this even compile?

No no, I'm creating a MediaStreamTrackPrivate with the given source and adding it to the tracks.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:60
> > +        videoTracks.append(MediaStreamTrackPrivate::create(stream->m_videoTracks[i]->source()));
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:77
> > +    return MediaStream::create(context, MediaStreamDescriptor::create(audioTracks, videoTracks));
> 
> Ditto.

Here I'm creating a MediaStreamDescriptor with the track vectors and passing it to the MediaStream::create method.

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:45
> > +PassRefPtr<MediaStreamDescriptor> MediaStreamDescriptor::create(const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources)
> 
> Nit: You can return RefPtr here. PassRefPtrs aren't needed since Anders added move-semantics to the RefPtr class.

OK.

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:50
> > +PassRefPtr<MediaStreamDescriptor> MediaStreamDescriptor::create(const MediaStreamTrackPrivateVector& audioPrivateTracks, const MediaStreamTrackPrivateVector& videoPrivateTracks)
> 
> Ditto.
> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:121
> > +    for (size_t i = 0; i < audioSources.size(); i++)
> > +        addTrackAndSource(MediaStreamTrackPrivate::create(audioSources[i]));
> > +
> > +    for (size_t i = 0; i < videoSources.size(); i++)
> > +        addTrackAndSource(MediaStreamTrackPrivate::create(videoSources[i]));
> > +}
> 
> Does m_ended need to be set if all sources have ended (readyState == Ended)?

First I thought to put this behaviour here too, since we create tracks for the passed sources. But, since the spec only states for tracks that are passed to the constructor I left it that way. Maybe we can put this here too. What do you think?

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:136
> > +    unsigned providedTracksSize = audioPrivateTracks.size() + videoPrivateTracks.size();
> > +    unsigned tracksSize = m_audioPrivateTracks.size() + m_videoPrivateTracks.size();
> 
> I don't think these local variables are necessary.

I just put them to improve code readability, otherwise the if condition would be too long: if ((audioPrivateTracks.size() + videoPrivateTracks.size()) > 0 &&  !(m_audioPrivateTracks.size() + m_videoPrivateTracks.size()))

What do you think?

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:163
> > +    for (size_t i = 0; i < sourceVector.size(); ++i) {
> > +        if (id == sourceVector[i]->id())
> > +            return true;
> >      }
> > +
> > +    return false;
> 
> Can you use sourceVector.find() here?

I could use that, but I don't if that would be safe enough, unless it is guaranteed that would never happen sources of the same id in different objects.

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:59
> > +    static PassRefPtr<MediaStreamDescriptor> create(const MediaStreamSourceVector& audioSources, const MediaStreamSourceVector& videoSources);
> > +    static PassRefPtr<MediaStreamDescriptor> create(const MediaStreamTrackPrivateVector& audioPrivateTracks, const MediaStreamTrackPrivateVector& videoPrivateTracks);
> 
> Nit: You can return RefPtr here. PassRefPtrs aren't needed since Anders added move-semantics to the RefPtr class.

OK.

> 
> > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h:119
> > +typedef Vector<RefPtr<MediaStreamTrackPrivate>> MediaStreamTrackPrivateVector;
> 
> This is unnecessary.

OK.

[Thiago de Barros Lacerda]

Changing MediaStreamDescriptor::create return to RefPtr will lead to some other modifications in MediaStream and whoever depends on them, and the code will be with half RefPtr and half PassRefPtr, which, in my opinion, will be very messy and confusing. I think this kind of modification must be done in a patch that changes all PassRefPtr to RefPtr of a given class.

[Eric Carlson]

(In reply to comment #11)
> Changing MediaStreamDescriptor::create return to RefPtr will lead to some other modifications in MediaStream and whoever depends on them, and the code will be with half RefPtr and half PassRefPtr, which, in my opinion, will be very messy and confusing. I think this kind of modification must be done in a patch that changes all PassRefPtr to RefPtr of a given class.

That seems fine.

[Thiago de Barros Lacerda]

Created attachment 215563 [details]
Updated patch

[Eric Carlson]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 215437 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=215437&action=review
> > 
> > 
> > > Source/WebCore/Modules/mediastream/MediaStream.cpp:57
> > > +        audioTracks.append(MediaStreamTrackPrivate::create(stream->m_audioTracks[i]->source()));
> > 
> > Why are you adding the track's *source* to the array of *tracks*? Does this even compile?
> 
> No no, I'm creating a MediaStreamTrackPrivate with the given source and adding it to the tracks.
> 
  Oops, you certainly are!

  I don't think creating new tracks is correct though, should't you just add the track as-it?

> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:50
> > > +PassRefPtr<MediaStreamDescriptor> MediaStreamDescriptor::create(const MediaStreamTrackPrivateVector& audioPrivateTracks, const MediaStreamTrackPrivateVector& videoPrivateTracks)
> > 
> > Ditto.
> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:121
> > > +    for (size_t i = 0; i < audioSources.size(); i++)
> > > +        addTrackAndSource(MediaStreamTrackPrivate::create(audioSources[i]));
> > > +
> > > +    for (size_t i = 0; i < videoSources.size(); i++)
> > > +        addTrackAndSource(MediaStreamTrackPrivate::create(videoSources[i]));
> > > +}
> > 
> > Does m_ended need to be set if all sources have ended (readyState == Ended)?
> 
> First I thought to put this behaviour here too, since we create tracks for the passed sources. But, since the spec only states for tracks that are passed to the constructor I left it that way. Maybe we can put this here too. What do you think?
> 
  I think it is a good idea because a stream has ended when all of its tracks have ended.


> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:136
> > > +    unsigned providedTracksSize = audioPrivateTracks.size() + videoPrivateTracks.size();
> > > +    unsigned tracksSize = m_audioPrivateTracks.size() + m_videoPrivateTracks.size();
> > 
> > I don't think these local variables are necessary.
> 
> I just put them to improve code readability, otherwise the if condition would be too long: if ((audioPrivateTracks.size() + videoPrivateTracks.size()) > 0 &&  !(m_audioPrivateTracks.size() + m_videoPrivateTracks.size()))
> 
> What do you think?
> 

  Your call.

> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:163
> > > +    for (size_t i = 0; i < sourceVector.size(); ++i) {
> > > +        if (id == sourceVector[i]->id())
> > > +            return true;
> > >      }
> > > +
> > > +    return false;
> > 
> > Can you use sourceVector.find() here?
> 
> I could use that, but I don't if that would be safe enough, unless it is guaranteed that would never happen sources of the same id in different objects.
> 

  It is an error if two sources have the same ID.

[Thiago de Barros Lacerda]

Created attachment 215576 [details]
Updated patch

[Thiago de Barros Lacerda]

Created attachment 215583 [details]
Final patch (I hope)

[Eric Carlson]

Comment on attachment 215583 [details]
Final patch (I hope)

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

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:59
> +    static PassRefPtr<MediaStreamDescriptor> create(const Vector<RefPtr<MediaStreamTrackPrivate>>& audioPrivateTracks, const Vector<RefPtr<MediaStreamTrackPrivate>>& videoPrivateTracks);

Nit: I think this can be made private, for now at least. This can be done in a follow-up.

[WebKit Commit Bot]

Comment on attachment 215583 [details]
Final patch (I hope)

Clearing flags on attachment: 215583

Committed r158337: <http://trac.webkit.org/changeset/158337>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.