Revert audioTimePitchAlgorithm to "TimeDomain" from "Spectral"
Created attachment 417038 [details] Patch
<rdar://problem/72829926>
Created attachment 417042 [details] Patch
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/
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?
(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.
(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.
> > > 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.
(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.
Created attachment 417106 [details] Patch for landing
ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.
Created attachment 417134 [details] Patch for landing
Created attachment 417135 [details] Patch for landing
Committed r271219: <https://trac.webkit.org/changeset/271219> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417135 [details].