Bug 168520 – Refactoring: Remove AudioSourceObserverObjC and AudioCaptureSourceProviderObjC

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Refactoring: Remove AudioSourceObserverObjC and AudioCaptureSourceProviderObjC

[Jer Noble]

Created attachment 301937 [details]
Patch

[Jer Noble]

Created attachment 301940 [details]
Patch

[Jer Noble]

Created attachment 301952 [details]
Patch

[Jer Noble]

Created attachment 301956 [details]
Patch

[youenn fablet]

Comment on attachment 301956 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        a lot of unnecessary conversion code and observer duplication.

Very nice!

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:63
>      : m_captureSource(&source)

Are we nullifying m_captureSource or can we make it a reference?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225
> +    if (!m_ringBuffer || !is<WebAudioBufferList>(data))

Can data be something else than a WebAudioBufferList?
If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if statement?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:230
> +    m_ringBuffer->store(bufferList.list(), frameCount, m_writeCount);

IIRC, bufferList.list() is this ugly audio structure.
Maybe we should try to limit its usage in the API or have a sanitising wrapper for it.

[Jer Noble]

(In reply to comment #5)
> Comment on attachment 301956 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301956&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        a lot of unnecessary conversion code and observer duplication.
> 
> Very nice!
> 
> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:63
> >      : m_captureSource(&source)
> 
> Are we nullifying m_captureSource or can we make it a reference?

We are not, but we probably should be in unprepare().

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225
> > +    if (!m_ringBuffer || !is<WebAudioBufferList>(data))
> 
> Can data be something else than a WebAudioBufferList?
> If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if
> statement?

Right now, it can be nothing other than a WebAudioBufferList. So we could.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:230
> > +    m_ringBuffer->store(bufferList.list(), frameCount, m_writeCount);
> 
> IIRC, bufferList.list() is this ugly audio structure.
> Maybe we should try to limit its usage in the API or have a sanitising
> wrapper for it.

We can't, because this API touches the non-mac/cocoa code, ad PlatformAudioData is the platform-independent way of passing these around.

[Jer Noble]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 301956 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=301956&action=review
> > 
> > > Source/WebCore/ChangeLog:17
> > > +        a lot of unnecessary conversion code and observer duplication.
> > 
> > Very nice!
> > 
> > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:63
> > >      : m_captureSource(&source)
> > 
> > Are we nullifying m_captureSource or can we make it a reference?
> 
> We are not, but we probably should be in unprepare().
> 
> > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225
> > > +    if (!m_ringBuffer || !is<WebAudioBufferList>(data))
> > 
> > Can data be something else than a WebAudioBufferList?
> > If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if
> > statement?
> 
> Right now, it can be nothing other than a WebAudioBufferList. So we could.
> 
> > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:230
> > > +    m_ringBuffer->store(bufferList.list(), frameCount, m_writeCount);
> > 
> > IIRC, bufferList.list() is this ugly audio structure.
> > Maybe we should try to limit its usage in the API or have a sanitising
> > wrapper for it.
> 
> We can't, because this API touches the non-mac/cocoa code, ad
> PlatformAudioData is the platform-independent way of passing these around.

I'll take care of these ASSERT additions and adding things to unprepared() in an upcoming patch.

[Jer Noble]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Comment on attachment 301956 [details]
> > > > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:225
> > > > +    if (!m_ringBuffer || !is<WebAudioBufferList>(data))
> > > 
> > > Can data be something else than a WebAudioBufferList?
> > > If so, should we add ASSERT(is<WebAudioBufferList>(data)) before this if
> > > statement?
> > 
> > Right now, it can be nothing other than a WebAudioBufferList. So we could.

It turns out that downcast<> already has an ASSERT_WITH_SECURITY_IMPLICATION() so I think all that's left to do here is remove the " || !is<...>" part.

[WebKit Commit Bot]

Comment on attachment 301956 [details]
Patch

Clearing flags on attachment: 301956

Committed r212572: <http://trac.webkit.org/changeset/212572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.