After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 382982 - [apedemux] Fails to read track gain or other doubles
[apedemux] Fails to read track gain or other doubles
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 0.10.5
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-12-06 12:28 UTC by Sebastian Dröge (slomo)
Modified: 2006-12-09 19:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
apedemux.diff (793 bytes, patch)
2006-12-06 12:29 UTC, Sebastian Dröge (slomo)
committed Details | Review
apedemux.diff (756 bytes, patch)
2006-12-08 06:56 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2006-12-06 12:28:19 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
Comment 1 Sebastian Dröge (slomo) 2006-12-06 12:29:00 UTC
Created attachment 77806 [details] [review]
apedemux.diff
Comment 2 Jan Schmidt 2006-12-06 13:17:39 UTC
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 >
Comment 3 David Schleef 2006-12-06 18:38:05 UTC
Shouldn't this be g_ascii_strtod()?
Comment 4 Sebastian Dröge (slomo) 2006-12-06 19:28:55 UTC
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 :)
Comment 5 Tim-Philipp Müller 2006-12-07 20:54:09 UTC
> 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).
Comment 6 Sebastian Dröge (slomo) 2006-12-08 06:56:29 UTC
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 ','.
Comment 7 Tim-Philipp Müller 2006-12-09 19:28:53 UTC
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.