Bug 204290 – Introduce a mock implementation of CoreAudioSharedUnit

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

[youenn fablet]

This will make CoreAudioCaptureSource testing feasible

[youenn fablet]

Created attachment 383733 [details]
Patch

[youenn fablet]

Created attachment 383817 [details]
Patch

[youenn fablet]

Created attachment 383829 [details]
Patch

[EWS Watchlist]

Comment on attachment 383829 [details]
Patch

Attachment 383829 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13266071

New failing tests:
imported/blink/fast/events/panScroll-crash.html

[EWS Watchlist]

Created attachment 383847 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[youenn fablet]

Comment on attachment 383847 [details]
Archive of layout-test-results from ews211 for win-future

Error unrelated

[Eric Carlson]

Comment on attachment 383829 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.h:71
> +    virtual CapabilityValueOrRange sampleRateCapacities() const { return CapabilityValueOrRange(8000, 96000); }

Nit: it is a bit strange to have this return a default value from the base class when other trivial methods (isProducingData, hasAudioUnit) do not. It would be cleaner to also make it pure virtual.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:896
> +    auto& unit = m_overrideUnit ? *m_overrideUnit : CoreAudioSharedUnit::singleton();
>      if (unit.isSuspended())

if (this->unit().isSuspended())

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:905
> +    if (m_overrideUnit)
> +        m_overrideUnit->delaySamples(seconds);

Maybe have the non-mock unit, or the base class, have an empty delaySamples method instead?

[youenn fablet]

Created attachment 383866 [details]
Patch for landing

[youenn fablet]

(In reply to Eric Carlson from comment #7)
> Comment on attachment 383829 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383829&action=review
> 
> > Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.h:71
> > +    virtual CapabilityValueOrRange sampleRateCapacities() const { return CapabilityValueOrRange(8000, 96000); }
> 
> Nit: it is a bit strange to have this return a default value from the base
> class when other trivial methods (isProducingData, hasAudioUnit) do not. It
> would be cleaner to also make it pure virtual.

OK

> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:896
> > +    auto& unit = m_overrideUnit ? *m_overrideUnit : CoreAudioSharedUnit::singleton();
> >      if (unit.isSuspended())
> 
> if (this->unit().isSuspended())

Issue was constness, I added a unit() const version.

> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:905
> > +    if (m_overrideUnit)
> > +        m_overrideUnit->delaySamples(seconds);
> 
> Maybe have the non-mock unit, or the base class, have an empty delaySamples
> method instead?

It already has so I changed it to unit().delaySamples.

[WebKit Commit Bot]

Comment on attachment 383866 [details]
Patch for landing

Clearing flags on attachment: 383866

Committed r252660: <https://trac.webkit.org/changeset/252660>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57342588>

[Aakash Jain]

> Committed r252660: <https://trac.webkit.org/changeset/252660>
This seems to have broken platform/ios/mediastream/audio-muted-in-background-tab.html
 
Tracked in Bug 204408.