GNOME Bugzilla – Bug 382982
[apedemux] Fails to read track gain or other doubles
Last modified: 2006-12-09 19:28:53 UTC
Hi, the attached patch allows apedemux to correctly parse track gain, album gain or other doubles, no matter what the current locale is. sscanf() only tries the current locale and thus breaks if for example the decimal point is another character than '.' g_strtod() parses using the locale settings and if this fails using the C locale settings, i.e. '.' for decimal point. This fixes the apev2mux unit test when running with de_DE for example. Bye
Created attachment 77806 [details] [review] apedemux.diff
Committed, ta: * gst/apetag/gstapedemux.c: (ape_demux_parse_tags): Use g_strtod() instead of sscanf to parse doubles, so that it will try parsing in the C locale if the current locale fails. Fixes: #382982 Patch by: Sebastian Dröge <mail at slomosnail de >
Shouldn't this be g_ascii_strtod()?
Relevant part of the IRC discussion: <MikeS> slomo: don't you want to ONLY parse with the C locale? <slomo> MikeS: not sure... before it accepted non-C-locale doubles so maybe someone has assumed this in his code or something... it's not really defined in the "spec" for ape * MikeS shrugs. <MikeS> ok. -good isn't frozen yet, just feature-frozen. that's a bugfix. <slomo> if you think it's better to use g_ascii_strtod() feel free to change it... but imho g_strtod() can't break here unless there's something completely broken in the tags... but then g_ascii_strtod() would most probably break too <thaytan> having a locale-dependant string makes no sense <thaytan> but as you say, it captures a superset, so no harm So if you have reasons why g_ascii_strtod() has any advantages over g_strtod() feel free to change it :)
> Shouldn't this be g_ascii_strtod()? Nah, it should really be g_strtod(). apedemux needs to be able to parse both 'value=1.234' and 'value=1,234' at all times, since the locale of the tag writer and the locale of the tag reader may be different and the format for this isn't well-defined in the ape spec AFAIK (but even if it was, chances that it would be implemented correctly by all tag writers are pretty slim so we'd still need to be able to parse both).
Created attachment 77947 [details] [review] apedemux.diff Well, it would probably better to use g_ascii_strtod() but replace all occurences of ',' with '.' before. g_strtod() would only parse ',' correctly if the current locale is one with ',' as decimal point. Attached is a patch that does this instead. Maybe we want other chars than ',' replaced but AFAIK the only chars used as decimal point are '.' and ','.
You're right, I misremembered what g_strtod() does, I thought it parsed either version: 2006-12-09 Tim-Philipp Müller <tim at centricular dot net> Patch by: Sebastian Dröge <mail at slomosnail de> * gst/apetag/gstapedemux.c: (ape_demux_parse_tags): We need to be able to read and parse any possible floating point string format ("1,234" or "1.234") irrespective of the current locale. g_strod() will parse the former only in certain locales though, so we really need to canonicalise the separator to '.' and then use g_ascii_strtod() to make sure we can parse either version at all times. Fixes #382982 for real.