Bug 123301 – Adding platform implementation of MediaStreamTrack

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

[Thiago de Barros Lacerda]

aka MediaStreamTrackPrivate

[Thiago de Barros Lacerda]

Created attachment 215121 [details]
Patch

[Eric Carlson]

Comment on attachment 215121 [details]
Patch

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

> Source/WebCore/ChangeLog:63
> +        * Modules/mediastream/AudioStreamTrack.cpp:
> +        (WebCore::AudioStreamTrack::create):
> +        (WebCore::AudioStreamTrack::AudioStreamTrack):
> +        * Modules/mediastream/AudioStreamTrack.h:
> +        * Modules/mediastream/MediaStream.cpp:
> +        (WebCore::MediaStream::MediaStream):
> +        (WebCore::MediaStream::addRemoteSource):
> +        * Modules/mediastream/MediaStreamTrack.cpp:
> +        (WebCore::MediaStreamTrack::MediaStreamTrack):
> +        (WebCore::MediaStreamTrack::~MediaStreamTrack):
> +        (WebCore::MediaStreamTrack::kind):
> +        (WebCore::MediaStreamTrack::setSource):
> +        (WebCore::MediaStreamTrack::id):
> +        (WebCore::MediaStreamTrack::label):
> +        (WebCore::MediaStreamTrack::enabled):
> +        (WebCore::MediaStreamTrack::setEnabled):
> +        (WebCore::MediaStreamTrack::muted):
> +        (WebCore::MediaStreamTrack::readonly):
> +        (WebCore::MediaStreamTrack::remote):
> +        (WebCore::MediaStreamTrack::readyState):
> +        (WebCore::MediaStreamTrack::states):
> +        (WebCore::MediaStreamTrack::capabilities):
> +        (WebCore::MediaStreamTrack::clone):
> +        (WebCore::MediaStreamTrack::stopProducingData):
> +        (WebCore::MediaStreamTrack::ended):
> +        (WebCore::MediaStreamTrack::sourceStateChanged):
> +        (WebCore::MediaStreamTrack::sourceMutedChanged):
> +        (WebCore::MediaStreamTrack::sourceEnabledChanged):
> +        (WebCore::MediaStreamTrack::configureTrackRendering):
> +        (WebCore::MediaStreamTrack::stopped):
> +        (WebCore::MediaStreamTrack::trackDidEnd):
> +        (WebCore::MediaStreamTrack::trackReadyStateChanged):
> +        (WebCore::MediaStreamTrack::trackMutedChanged):
> +        * Modules/mediastream/MediaStreamTrack.h:
> +        (WebCore::MediaStreamTrack::source):
> +        (WebCore::MediaStreamTrack::descriptor):
> +        * Modules/mediastream/VideoStreamTrack.cpp:
> +        (WebCore::VideoStreamTrack::create):
> +        (WebCore::VideoStreamTrack::VideoStreamTrack):
> +        * Modules/mediastream/VideoStreamTrack.h:
> +        * platform/mediastream/MediaStreamDescriptor.cpp:
> +        (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> +        (WebCore::MediaStreamDescriptor::addTrack):
> +        (WebCore::MediaStreamDescriptor::removeTrack):
> +        * platform/mediastream/MediaStreamDescriptor.h:
> +        (WebCore::MediaStreamDescriptor::numberOfAudioSources):
> +        (WebCore::MediaStreamDescriptor::audioSources):
> +        (WebCore::MediaStreamDescriptor::numberOfVideoSources):
> +        (WebCore::MediaStreamDescriptor::videoSources):
> +        (WebCore::MediaStreamDescriptor::numberOfAudioTracks):
> +        (WebCore::MediaStreamDescriptor::audioTracks):
> +        (WebCore::MediaStreamDescriptor::numberOfVideoTracks):
> +        (WebCore::MediaStreamDescriptor::videoTracks):

I would prefer to see brief comments about what changed in each method.

> Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:41
> +    RefPtr<MediaStreamTrackPrivate> descriptor = MediaStreamTrackPrivate::create(0);
> +    return adoptRef(new AudioStreamTrack(context, descriptor.get(), &audioConstraints));

Nit: the local variable is unnecessary.

> Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:54
> +AudioStreamTrack::AudioStreamTrack(ScriptExecutionContext* context, MediaStreamTrackPrivate* descriptor, const Dictionary* audioConstraints)

The private track is stored in the base class as a RefPtr<MediaStreamTrackPrivate>, why pass it as a MediaStreamTrackPrivate* (other than that is what the code did with the source :-) )?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:56
> +    , m_descriptor(descriptor)

Nit: "m_descriptor" isn't a great name now that you have changed the name of the class (thanks for that!). I suggest something like "m_privateTrack".

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:272
>      for (Vector<Observer*>::iterator i = m_observers.begin(); i != m_observers.end(); ++i)
>          (*i)->trackDidEnd();
> +
> +    m_descriptor->setReadyState(MediaStreamSource::Ended);

Should the private track readyState be set before calling the observers?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:98
> +    // MediaStreamTrackDescriptorClient

MediaStreamTrackDescriptorClient -> MediaStreamTrackPrivateClient

> Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp:40
> +    RefPtr<MediaStreamTrackPrivate> descriptor = MediaStreamTrackPrivate::create(0);

Nit: the local variable is unnecessary.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109
>          m_audioStreamSources.append(audioSources[i]);
> +        m_audioTrackDescriptors.append(MediaStreamTrackPrivate::create(audioSources[i]));

Is m_audioStreamSources necessary, can't we use trackPrivate->source()?

m_audioTrackDescriptors isn't a great name not that the class name has changed.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:174
> +    if (source())
> +        source()->setEnabled(enabled);

This is not correct, we don't necessarily want to disable a shared source. We don't have enough infrastructure in place yet to fix this correctly, so why don't you just remove it an add a FIXME and we can follow up with a fix in another patch.

[Thiago de Barros Lacerda]

(In reply to comment #2)
> (From update of attachment 215121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215121&action=review
> 
> > Source/WebCore/ChangeLog:63
> > +        * Modules/mediastream/AudioStreamTrack.cpp:
> > +        (WebCore::AudioStreamTrack::create):
> > +        (WebCore::AudioStreamTrack::AudioStreamTrack):
> > +        * Modules/mediastream/AudioStreamTrack.h:
> > +        * Modules/mediastream/MediaStream.cpp:
> > +        (WebCore::MediaStream::MediaStream):
> > +        (WebCore::MediaStream::addRemoteSource):
> > +        * Modules/mediastream/MediaStreamTrack.cpp:
> > +        (WebCore::MediaStreamTrack::MediaStreamTrack):
> > +        (WebCore::MediaStreamTrack::~MediaStreamTrack):
> > +        (WebCore::MediaStreamTrack::kind):
> > +        (WebCore::MediaStreamTrack::setSource):
> > +        (WebCore::MediaStreamTrack::id):
> > +        (WebCore::MediaStreamTrack::label):
> > +        (WebCore::MediaStreamTrack::enabled):
> > +        (WebCore::MediaStreamTrack::setEnabled):
> > +        (WebCore::MediaStreamTrack::muted):
> > +        (WebCore::MediaStreamTrack::readonly):
> > +        (WebCore::MediaStreamTrack::remote):
> > +        (WebCore::MediaStreamTrack::readyState):
> > +        (WebCore::MediaStreamTrack::states):
> > +        (WebCore::MediaStreamTrack::capabilities):
> > +        (WebCore::MediaStreamTrack::clone):
> > +        (WebCore::MediaStreamTrack::stopProducingData):
> > +        (WebCore::MediaStreamTrack::ended):
> > +        (WebCore::MediaStreamTrack::sourceStateChanged):
> > +        (WebCore::MediaStreamTrack::sourceMutedChanged):
> > +        (WebCore::MediaStreamTrack::sourceEnabledChanged):
> > +        (WebCore::MediaStreamTrack::configureTrackRendering):
> > +        (WebCore::MediaStreamTrack::stopped):
> > +        (WebCore::MediaStreamTrack::trackDidEnd):
> > +        (WebCore::MediaStreamTrack::trackReadyStateChanged):
> > +        (WebCore::MediaStreamTrack::trackMutedChanged):
> > +        * Modules/mediastream/MediaStreamTrack.h:
> > +        (WebCore::MediaStreamTrack::source):
> > +        (WebCore::MediaStreamTrack::descriptor):
> > +        * Modules/mediastream/VideoStreamTrack.cpp:
> > +        (WebCore::VideoStreamTrack::create):
> > +        (WebCore::VideoStreamTrack::VideoStreamTrack):
> > +        * Modules/mediastream/VideoStreamTrack.h:
> > +        * platform/mediastream/MediaStreamDescriptor.cpp:
> > +        (WebCore::MediaStreamDescriptor::MediaStreamDescriptor):
> > +        (WebCore::MediaStreamDescriptor::addTrack):
> > +        (WebCore::MediaStreamDescriptor::removeTrack):
> > +        * platform/mediastream/MediaStreamDescriptor.h:
> > +        (WebCore::MediaStreamDescriptor::numberOfAudioSources):
> > +        (WebCore::MediaStreamDescriptor::audioSources):
> > +        (WebCore::MediaStreamDescriptor::numberOfVideoSources):
> > +        (WebCore::MediaStreamDescriptor::videoSources):
> > +        (WebCore::MediaStreamDescriptor::numberOfAudioTracks):
> > +        (WebCore::MediaStreamDescriptor::audioTracks):
> > +        (WebCore::MediaStreamDescriptor::numberOfVideoTracks):
> > +        (WebCore::MediaStreamDescriptor::videoTracks):
> 
> I would prefer to see brief comments about what changed in each method.
> 

OK

> > Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:41
> > +    RefPtr<MediaStreamTrackPrivate> descriptor = MediaStreamTrackPrivate::create(0);
> > +    return adoptRef(new AudioStreamTrack(context, descriptor.get(), &audioConstraints));
> 
> Nit: the local variable is unnecessary.

Indeed

> 
> > Source/WebCore/Modules/mediastream/AudioStreamTrack.cpp:54
> > +AudioStreamTrack::AudioStreamTrack(ScriptExecutionContext* context, MediaStreamTrackPrivate* descriptor, const Dictionary* audioConstraints)
> 
> The private track is stored in the base class as a RefPtr<MediaStreamTrackPrivate>, why pass it as a MediaStreamTrackPrivate* (other than that is what the code did with the source :-) )?

Makes sense

> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:56
> > +    , m_descriptor(descriptor)
> 
> Nit: "m_descriptor" isn't a great name now that you have changed the name of the class (thanks for that!). I suggest something like "m_privateTrack".

remains of my old patch :(

> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:272
> >      for (Vector<Observer*>::iterator i = m_observers.begin(); i != m_observers.end(); ++i)
> >          (*i)->trackDidEnd();
> > +
> > +    m_descriptor->setReadyState(MediaStreamSource::Ended);
> 
> Should the private track readyState be set before calling the observers?

you are right

> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.h:98
> > +    // MediaStreamTrackDescriptorClient
> 
> MediaStreamTrackDescriptorClient -> MediaStreamTrackPrivateClient

Ooops

> 
> > Source/WebCore/Modules/mediastream/VideoStreamTrack.cpp:40
> > +    RefPtr<MediaStreamTrackPrivate> descriptor = MediaStreamTrackPrivate::create(0);
> 
> Nit: the local variable is unnecessary.

OK

> 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109
> >          m_audioStreamSources.append(audioSources[i]);
> > +        m_audioTrackDescriptors.append(MediaStreamTrackPrivate::create(audioSources[i]));
> 
> Is m_audioStreamSources necessary, can't we use trackPrivate->source()?
> 
I think it is better to keep them, since we have methods to remote and add sources. It is better to iterate through them then in each track and use the source() method (remember that can be shared sources between tracks)

> m_audioTrackDescriptors isn't a great name not that the class name has changed.

oops, again remains of my old patch :(
m_audioPrivateTracks?

> 
> > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:174
> > +    if (source())
> > +        source()->setEnabled(enabled);
> 
> This is not correct, we don't necessarily want to disable a shared source. We don't have enough infrastructure in place yet to fix this correctly, so why don't you just remove it an add a FIXME and we can follow up with a fix in another patch.

I shall pay more attention, this is again remains of my old patch, when trunk did not have my patch that lets a source to be shared.
I think this should only set the enabled property of the track. Nothing should be done to the source. Didn't get what you mean that we can additionally do here.

[Eric Carlson]

(In reply to comment #3)
> (In reply to comment #2)
> > 
> > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109
> > >          m_audioStreamSources.append(audioSources[i]);
> > > +        m_audioTrackDescriptors.append(MediaStreamTrackPrivate::create(audioSources[i]));
> > 
> > Is m_audioStreamSources necessary, can't we use trackPrivate->source()?
> > 
> I think it is better to keep them, since we have methods to remote and add sources. It is better to iterate through them then in each track and use the source() method (remember that can be shared sources between tracks)
> 
  Ah, I forgot about MediaStream::removeRemoteSource!

> > m_audioTrackDescriptors isn't a great name not that the class name has changed.
> 
> oops, again remains of my old patch :(
> m_audioPrivateTracks?
> 
  Sure.

> > 
> > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:174
> > > +    if (source())
> > > +        source()->setEnabled(enabled);
> > 
> > This is not correct, we don't necessarily want to disable a shared source. We don't have enough infrastructure in place yet to fix this correctly, so why don't you just remove it an add a FIXME and we can follow up with a fix in another patch.
> 
> I shall pay more attention, this is again remains of my old patch, when trunk did not have my patch that lets a source to be shared.
> I think this should only set the enabled property of the track. Nothing should be done to the source. Didn't get what you mean that we can additionally do here.
>
  We need to inform the source that the track isn't using it so it can stop doing work. For example, we want a local source to stop using the camera or microphone as soon as there are no active tracks. 

  Now that I think about it, I think calling "source()->setEnabled(enabled)" here is the right thing to do. We just need to make sure MediaStreamSource:: setEnabled does something like MediaStreamSource:: stop does, and call a virtual source method when there are no active tracks. I will take care of this in the patch for bug 123316 I will post later today.

[Thiago de Barros Lacerda]

Created attachment 215187 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215187 [details]
Patch

Clearing flags on attachment: 215187

Committed r158018: <http://trac.webkit.org/changeset/158018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.