Bug 211718 – Introduce a RealtimeMediaSource video sample observer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Introduce a RealtimeMediaSource video sample observer

[youenn fablet]

Created attachment 399008 [details]
Patch

[youenn fablet]

Created attachment 399017 [details]
Patch

[Eric Carlson]

Comment on attachment 399017 [details]
Patch

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

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107
>      if (m_audioSource)
>          m_audioSource->removeAudioSampleObserver(*this);
> +    if (m_videoSource)
> +        m_videoSource->removeVideoSampleObserver(*this);

Maybe add a FIXME about removing these, since they should never be necessary?

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210
> +    auto locker = holdLock(m_videoSampleObserversLock);
> +    for (auto* observer : m_videoSampleObservers)
> +        observer->videoSampleAvailable(mediaSample);

Clients may perform an arbitrary amount of processing during a callback, so holding the lock for the duration of this loop might be asking for trouble.

[Philippe Normand]

Thanks for keeping the GStreamer bits compiling!

[youenn fablet]

(In reply to Philippe Normand from comment #4)
> Thanks for keeping the GStreamer bits compiling!

np, note that the following step might be to call videoSampleAvailable from a background thread.
While this may not break compilation, this might require some GTK changes.

[youenn fablet]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 399017 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399017&action=review
> 
> > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107
> >      if (m_audioSource)
> >          m_audioSource->removeAudioSampleObserver(*this);
> > +    if (m_videoSource)
> > +        m_videoSource->removeVideoSampleObserver(*this);
> 
> Maybe add a FIXME about removing these, since they should never be necessary?

I added them as a just-in-case, maybe we should just keep the ASSERT.
The child class should really do it since otherwise, a half-destructed object might still receive samples. 

> > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210
> > +    auto locker = holdLock(m_videoSampleObserversLock);
> > +    for (auto* observer : m_videoSampleObservers)
> > +        observer->videoSampleAvailable(mediaSample);
> 
> Clients may perform an arbitrary amount of processing during a callback, so
> holding the lock for the duration of this loop might be asking for trouble.

This is the same design as for audio samples.
I think it is fine like it is since receiving video samples should not trigger adding/removing observers, and hopefully these actions will be in different threads.

[youenn fablet]

Comment on attachment 399017 [details]
Patch

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

>>> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107
>>> +        m_videoSource->removeVideoSampleObserver(*this);
>> 
>> Maybe add a FIXME about removing these, since they should never be necessary?
> 
> I added them as a just-in-case, maybe we should just keep the ASSERT.
> The child class should really do it since otherwise, a half-destructed object might still receive samples.

The child class destructor should do that early on otherwise, the object, while half destructed, might receive samples to process.
I'll add a comment.

>>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210
>>> +        observer->videoSampleAvailable(mediaSample);
>> 
>> Clients may perform an arbitrary amount of processing during a callback, so holding the lock for the duration of this loop might be asking for trouble.
> 
> This is the same design as for audio samples.
> I think it is fine like it is since receiving video samples should not trigger adding/removing observers, and hopefully these actions will be in different threads.

This lock is locked in add/remove observers which should happen in the main thread while this one should happen in a background thread.
Normally, videoSampleAvailable should never synchronously add/remove observers.

[Víctor M. Jáquez L.]

(In reply to youenn fablet from comment #5)
> (In reply to Philippe Normand from comment #4)
> > Thanks for keeping the GStreamer bits compiling!
> 
> np, note that the following step might be to call videoSampleAvailable from
> a background thread.
> While this may not break compilation, this might require some GTK changes.

Indeed, thanks. I'll look this patch today.

[youenn fablet]

Created attachment 399111 [details]
Patch for landing

[EWS]

Committed r261553: <https://trac.webkit.org/changeset/261553>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399111 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63131874>