aka MediaStreamTrackPrivate
Created attachment 215121 [details] Patch
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.
(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.
(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.
Created attachment 215187 [details] Patch
Comment on attachment 215187 [details] Patch Clearing flags on attachment: 215187 Committed r158018: <http://trac.webkit.org/changeset/158018>
All reviewed patches have been landed. Closing bug.