Bug 167639 – [Mac] Add an audio stream description class

• Status: ASSIGNED
• Component: Media

[Eric Carlson]

Create a class to wrap a CA audio stream description.

[Eric Carlson]

Created attachment 300219 [details]
Proposed patch

[youenn fablet]

Comment on attachment 300219 [details]
Proposed patch

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

> Source/WebCore/platform/audio/AudioStreamDescription.h:39
> +    } description;

Can we use Variant<std::nullptr_t, const AudioStreamBasicDescription*>?

> Source/WebCore/platform/audio/AudioStreamDescription.h:49
> +        Unknown,

Do we need Unknown and isPCM?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:115
> +            return Unknown;

Why do we return Unknown here? Shouldn't we return m_format?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:125
> +    return m_format;

Would it be clearer if we were returning early whenever we know the format.
Then at the end, we would return Unknown I guess?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:36
> +class CAAudioStreamDescription : public AudioStreamDescription {

Can it be final?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:41
> +    const PlatformDescription& platformDescription() const final;

Make all of these private?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:47
> +    bool isPCM() const final { return m_streamDescription.mFormatID == kAudioFormatLinearPCM; }

Do we need to override this one?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:64
> +    bool operator!=(const AudioStreamBasicDescription& other) { return !operator==(other); }

space around ==?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:66
> +    bool operator!=(const AudioStreamDescription& other) { return !operator==(other); }

Ditto?

> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:78
> +CAAudioStreamDescription toCAAudioStreamDescription(const AudioStreamDescription&);

Could be inlined maybe?

[Eric Carlson]

Comment on attachment 300219 [details]
Proposed patch

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

>> Source/WebCore/platform/audio/AudioStreamDescription.h:39
>> +    } description;
> 
> Can we use Variant<std::nullptr_t, const AudioStreamBasicDescription*>?

Yes, done.

>> Source/WebCore/platform/audio/AudioStreamDescription.h:49
>> +        Unknown,
> 
> Do we need Unknown and isPCM?

"None" is probably better than "Unknown"

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:115
>> +            return Unknown;
> 
> Why do we return Unknown here? Shouldn't we return m_format?

Oops!

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:125
>> +    return m_format;
> 
> Would it be clearer if we were returning early whenever we know the format.
> Then at the end, we would return Unknown I guess?

OK, fixed.

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:36
>> +class CAAudioStreamDescription : public AudioStreamDescription {
> 
> Can it be final?

Yes.

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:41
>> +    const PlatformDescription& platformDescription() const final;
> 
> Make all of these private?

No, because we will use this class directly in a number of places.

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:47
>> +    bool isPCM() const final { return m_streamDescription.mFormatID == kAudioFormatLinearPCM; }
> 
> Do we need to override this one?

No, fixed.

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:64
>> +    bool operator!=(const AudioStreamBasicDescription& other) { return !operator==(other); }
> 
> space around ==?

Fixed.

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:66
>> +    bool operator!=(const AudioStreamDescription& other) { return !operator==(other); }
> 
> Ditto?

Fixed.

>> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:78
>> +CAAudioStreamDescription toCAAudioStreamDescription(const AudioStreamDescription&);
> 
> Could be inlined maybe?

Good idea, fixed.

[Eric Carlson]

Created attachment 300226 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 300226 [details]
Patch for landing.

Clearing flags on attachment: 300226

Committed r211437: <http://trac.webkit.org/changeset/211437>