Bug 168520

Summary:

Refactoring: Remove AudioSourceObserverObjC and AudioCaptureSourceProviderObjC

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

New Bugs

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric.carlson, youennf

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

168412

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Jer Noble

Reported 2017-02-17 08:54:05 PST

Refactoring: Remove AudioSourceObserverObjC and AudioCaptureSourceProviderObjC

Attachments
Patch (36.48 KB, patch) 2017-02-17 09:00 PST, Jer Noble	no flags	Details Formatted Diff Diff
Patch (36.60 KB, patch) 2017-02-17 09:14 PST, Jer Noble	no flags	Details Formatted Diff Diff
Patch (34.65 KB, patch) 2017-02-17 10:21 PST, Jer Noble	no flags	Details Formatted Diff Diff
Patch (34.58 KB, patch) 2017-02-17 10:52 PST, Jer Noble	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jer Noble

Comment 1 2017-02-17 09:00:08 PST

Created attachment 301937 [details] Patch

Jer Noble

Comment 2 2017-02-17 09:14:40 PST

Created attachment 301940 [details] Patch

Jer Noble

Comment 3 2017-02-17 10:21:30 PST

Created attachment 301952 [details] Patch

Jer Noble

Comment 4 2017-02-17 10:52:03 PST

Created attachment 301956 [details] Patch

youenn fablet

Comment 5 2017-02-17 12:10:35 PST

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

Comment 6 2017-02-17 12:48:10 PST

(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

Comment 7 2017-02-17 13:05:51 PST

(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

Comment 8 2017-02-17 13:13:22 PST

(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 9 2017-02-17 13:35:21 PST

Comment on attachment 301956 [details] Patch Clearing flags on attachment: 301956 Committed r212572: <http://trac.webkit.org/changeset/212572>

WebKit Commit Bot

Comment 10 2017-02-17 13:35:25 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.