Bug 220341 – [Cocoa] Revert audioTimePitchAlgorithm to "TimeDomain" from "Spectral"

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Revert audioTimePitchAlgorithm to "TimeDomain" from "Spectral"

[Jer Noble]

Created attachment 417038 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/72829926>

[Jer Noble]

Created attachment 417042 [details]
Patch

[Peng Liu]

Comment on attachment 417042 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.h:402
> +    PitchCorrectionAlgorithm pitchCorrectionAlgorithm() { return m_pitchCorrectionAlgorithm; }

A const function?

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:291
> +#define WebKitPitchCorrectionAlgorithmPreferenceKey @"WebKitPitchCorrectionAlgorithmKey"

Nit. To be consistent with other keys, maybe
s/WebKitPitchCorrectionAlgorithmKey/WebKitPitchCorrectionAlgorithm/

[youenn fablet]

Comment on attachment 417042 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Revert to "TimeDomain", which is both more computationally inexpensive, and both propogates

s/propogates/propagates/

> Source/WTF/Scripts/Preferences/WebPreferences.yaml:1722
> +  webcoreBinding: custom

Can we remove that one so that we let WebPage::updateSettingsGenerated update the corresponding settings directly?
Or maybe it is due to the Settings being typed as MediaPlayerEnums::PitchCorrectionAlgorithm.
Might be nice for the pref generator to handle this code without static_cast below.
That might remove some webcoreBinding custom code as well.
Maybe Sam has some ideas here?

> Source/WebCore/html/HTMLMediaElement.cpp:1450
> +    m_player->setPitchCorrectionAlgorithm(static_cast<MediaPlayer::PitchCorrectionAlgorithm>(document().settings().pitchCorrectionAlgorithm()));

Do we really need this static_cast?

[Jer Noble]

(In reply to Peng Liu from comment #4)
> Comment on attachment 417042 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417042&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:402
> > +    PitchCorrectionAlgorithm pitchCorrectionAlgorithm() { return m_pitchCorrectionAlgorithm; }
> 
> A const function?

Good idea. Will change.

> > Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:291
> > +#define WebKitPitchCorrectionAlgorithmPreferenceKey @"WebKitPitchCorrectionAlgorithmKey"
> 
> Nit. To be consistent with other keys, maybe
> s/WebKitPitchCorrectionAlgorithmKey/WebKitPitchCorrectionAlgorithm/

Ok.

[Jer Noble]

(In reply to youenn fablet from comment #5)
> Comment on attachment 417042 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417042&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Revert to "TimeDomain", which is both more computationally inexpensive, and both propogates
> 
> s/propogates/propagates/

Will change.

> > Source/WTF/Scripts/Preferences/WebPreferences.yaml:1722
> > +  webcoreBinding: custom
> 
> Can we remove that one so that we let WebPage::updateSettingsGenerated
> update the corresponding settings directly?
> Or maybe it is due to the Settings being typed as
> MediaPlayerEnums::PitchCorrectionAlgorithm.
> Might be nice for the pref generator to handle this code without static_cast
> below.
> That might remove some webcoreBinding custom code as well.
> Maybe Sam has some ideas here?

There's an existing FIXME about adding enum support to the settings binding generator code. I decided not to tackle that bindings generator fix in this patch.

> > Source/WebCore/html/HTMLMediaElement.cpp:1450
> > +    m_player->setPitchCorrectionAlgorithm(static_cast<MediaPlayer::PitchCorrectionAlgorithm>(document().settings().pitchCorrectionAlgorithm()));
> 
> Do we really need this static_cast?

Yep, without it, the compiler issues a warning/error about narrowing a uint32_t to an enum.

[youenn fablet]

> > > Source/WebCore/html/HTMLMediaElement.cpp:1450
> > > +    m_player->setPitchCorrectionAlgorithm(static_cast<MediaPlayer::PitchCorrectionAlgorithm>(document().settings().pitchCorrectionAlgorithm()));
> > 
> > Do we really need this static_cast?
> 
> Yep, without it, the compiler issues a warning/error about narrowing a
> uint32_t to an enum.

I would have thought document().settings().pitchCorrectionAlgorithm() to return the value as an enum and not as a uint32_t.
The Settings generator should be able to do that.

[Jer Noble]

(In reply to youenn fablet from comment #8)
> > > > Source/WebCore/html/HTMLMediaElement.cpp:1450
> > > > +    m_player->setPitchCorrectionAlgorithm(static_cast<MediaPlayer::PitchCorrectionAlgorithm>(document().settings().pitchCorrectionAlgorithm()));
> > > 
> > > Do we really need this static_cast?
> > 
> > Yep, without it, the compiler issues a warning/error about narrowing a
> > uint32_t to an enum.
> 
> I would have thought document().settings().pitchCorrectionAlgorithm() to
> return the value as an enum and not as a uint32_t.
> The Settings generator should be able to do that.

I was wrong! Just built without the static_cast and no warning/error.

[Jer Noble]

Created attachment 417106 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

[Jer Noble]

Created attachment 417134 [details]
Patch for landing

[Jer Noble]

Created attachment 417135 [details]
Patch for landing

[EWS]

Committed r271219: <https://trac.webkit.org/changeset/271219>

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