Bug 217552 – Clean up CAAudioStreamDescription

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

Clean up CAAudioStreamDescription

[Peng Liu]

Created attachment 411004 [details]
Patch

[Peng Liu]

Created attachment 411020 [details]
Fix crashes in layout tests

[youenn fablet]

Comment on attachment 411020 [details]
Fix crashes in layout tests

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

> Source/WebCore/ChangeLog:9
> +        constructor and assignment operator because of the updates on the two getters.

Can you describe the benefit of this change?

> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:117
>      return m_platformDescription;

This is a potential change of behavior.
m_streamDescription could for instance be modified (for instance using streamDescription() non const getter).
This would make m_platformDescription out of sync.

> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122
> +    return m_format;

Why removing the lazy init of m_format?

[Peng Liu]

Comment on attachment 411020 [details]
Fix crashes in layout tests

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

>> Source/WebCore/ChangeLog:9
>> +        constructor and assignment operator because of the updates on the two getters.
> 
> Can you describe the benefit of this change?

Sure.
I encountered some weirdness(linker errors) when I tried to move the WEBCORE_EXPORT from the class to methods, so I took a closer look and found this class looks strange. The class defines the function calculateFormat() but does not provide an implementation, and CAAudioStreamDescription::platformDescription() always needs to update m_platformDescription.

>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:117
>>      return m_platformDescription;
> 
> This is a potential change of behavior.
> m_streamDescription could for instance be modified (for instance using streamDescription() non const getter).
> This would make m_platformDescription out of sync.

Oops, sounds not a good encapsulation. Should we disable that possibility?

>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122
>> +    return m_format;
> 
> Why removing the lazy init of m_format?

So it is just a lazy init? I thought we may need to do some unnecessary work whenever format() is called.

[youenn fablet]

Comment on attachment 411020 [details]
Fix crashes in layout tests

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

>>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:117
>>>      return m_platformDescription;
>> 
>> This is a potential change of behavior.
>> m_streamDescription could for instance be modified (for instance using streamDescription() non const getter).
>> This would make m_platformDescription out of sync.
> 
> Oops, sounds not a good encapsulation. Should we disable that possibility?

Actually, I was wrong here since we pass m_streamDescription as a pointer and not by value.
If we can remove streamDescription() non const getter, we should probably do it in any case.

I am not sure platformDescription is called much, so a lazy init seems fine here.

>>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122
>>> +    return m_format;
>> 
>> Why removing the lazy init of m_format?
> 
> So it is just a lazy init? I thought we may need to do some unnecessary work whenever format() is called.

It is a half lazy init. Agreed, it is a bit weird also since m_streamDescription could change but we would not change m_format accordingly if m_format is not None.
I guess that to make it clearer, we could use an Optional<Format>.

[Radar WebKit Bug Importer]

<rdar://problem/70401637>

[Peng Liu]

Comment on attachment 411020 [details]
Fix crashes in layout tests

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

>>>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122
>>>> +    return m_format;
>>> 
>>> Why removing the lazy init of m_format?
>> 
>> So it is just a lazy init? I thought we may need to do some unnecessary work whenever format() is called.
> 
> It is a half lazy init. Agreed, it is a bit weird also since m_streamDescription could change but we would not change m_format accordingly if m_format is not None.
> I guess that to make it clearer, we could use an Optional<Format>.

Yeah, seems better to use an Optional<>. However, it won't solve the problem when m_streamDescription is changed by other objects.

[Peng Liu]

Created attachment 412215 [details]
Patch

[Peng Liu]

Looks like it is not a good idea to refactor the CAAudioStreamDescription class now. Let’s remove the unused private function and close this bug.

[EWS]

Committed r268988: <https://trac.webkit.org/changeset/268988>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412215 [details].