[Webkit-unassigned] [Bug 123301] Adding platform implementation of MediaStreamTrack

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123301





--- Comment #3 from Thiago de Barros Lacerda <thiago.lacerda at openbossa.org>  2013-10-25 07:31:43 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.