Summary: | [Cocoa] Revert audioTimePitchAlgorithm to "TimeDomain" from "Spectral" | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, peng.liu6, philipj, sam, sergio, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=220447 | ||||||||||||||
Attachments: |
|
Description
Jer Noble
2021-01-05 14:33:08 PST
Created attachment 417038 [details]
Patch
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]. |