Bug 204290 - Introduce a mock implementation of CoreAudioSharedUnit
Summary: Introduce a mock implementation of CoreAudioSharedUnit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-18 00:13 PST by youenn fablet
Modified: 2019-11-20 04:59 PST (History)
14 users (show)

See Also:


Attachments
Patch (66.16 KB, patch)
2019-11-18 00:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (66.16 KB, patch)
2019-11-18 17:33 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (66.34 KB, patch)
2019-11-18 18:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (14.31 MB, application/zip)
2019-11-18 23:24 PST, EWS Watchlist
no flags Details
Patch for landing (66.63 KB, patch)
2019-11-19 07:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-11-18 00:13:21 PST
This will make CoreAudioCaptureSource testing feasible
Comment 1 youenn fablet 2019-11-18 00:20:41 PST
Created attachment 383733 [details]
Patch
Comment 2 youenn fablet 2019-11-18 17:33:22 PST
Created attachment 383817 [details]
Patch
Comment 3 youenn fablet 2019-11-18 18:04:09 PST
Created attachment 383829 [details]
Patch
Comment 4 EWS Watchlist 2019-11-18 23:24:00 PST
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
Comment 5 EWS Watchlist 2019-11-18 23:24:02 PST
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
Comment 6 youenn fablet 2019-11-19 00:21:09 PST
Comment on attachment 383847 [details]
Archive of layout-test-results from ews211 for win-future

Error unrelated
Comment 7 Eric Carlson 2019-11-19 06:07:02 PST
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?
Comment 8 youenn fablet 2019-11-19 07:22:23 PST
Created attachment 383866 [details]
Patch for landing
Comment 9 youenn fablet 2019-11-19 07:25:06 PST
(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.
Comment 10 WebKit Commit Bot 2019-11-19 16:50:58 PST
Comment on attachment 383866 [details]
Patch for landing

Clearing flags on attachment: 383866

Committed r252660: <https://trac.webkit.org/changeset/252660>
Comment 11 WebKit Commit Bot 2019-11-19 16:51:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-11-19 16:51:20 PST
<rdar://problem/57342588>
Comment 13 Aakash Jain 2019-11-20 04:59:20 PST
> 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.