#795 is one instance of a wider pattern. The worst instance is duration.
Duration
FfprobeMetadataMapper parses format.duration out of ffprobe's JSON with no format provider:
|
if (fmt.TryGetProperty("duration", out var durEl) |
|
&& durEl.ValueKind == JsonValueKind.String |
|
&& double.TryParse(durEl.GetString(), out var dur)) |
|
{ |
|
metadata.Duration = TimeSpan.FromSeconds(dur); |
ffprobe emits that field as a string and always uses ., regardless of the locale it runs under. I checked rather than assumed, in a container with de_DE.UTF-8 actually generated and awk printing 1,50 alongside it to prove the locale was live:
ffprobe under C: "duration": "1.071020"
ffprobe under de_DE.UTF-8: "duration": "1.071020"
The value does not change. What the parse makes of it does:
| book |
value from ffprobe |
de-DE |
fr-FR |
| one hour |
3600.5 |
36005 |
fails to parse |
| twelve hours |
43200.250 |
43200250 |
fails to parse |
. is the group separator in de-DE, so the decimal point is silently eaten and a twelve hour book is recorded as 43,200,250 seconds. In fr-FR the parse fails and the duration is lost. That value is persisted: it becomes DurationSeconds on the file record through LibraryManualScanWorkflow, AudiobookFileService, MetadataRescanService and AdminMetadataController.
Who this affects
Same precondition as #795, and worth stating before the list below. A stock container runs under the invariant culture and is unaffected. It takes a real culture reaching the process, which happens when LANG or LC_ALL is set, and on a desktop install that inherits the OS locale. I have not tested the Windows or macOS builds and am not claiming anything about them.
The other sites
Sizes, all parsed from strings that external services emit with .:
Under de-DE a 1.5 GB release is read as 15 GB; under fr-FR the parse fails and the size is zero.
Two more are already covered by existing work: Audiobook.cs:108 and DownloadImportService.cs:371 are what #763 fixes, and AudibleSeriesWorkflow.cs:342 is #795.
Counting only decimal, double and DateTime, and excluding tests and mocks, canary has 27 parses with no format provider and 5 that pass one. I am deliberately not claiming the DateTime ones are broken: ISO 8601 and RFC822 both parse correctly under every culture I tried, and the only failure I could produce needed an ambiguous numeric date, where the right answer depends on what the source emits.
Already correct in this codebase
NzbgetHistoryReader.cs:157 parses megabytes with NumberStyles.Float, CultureInfo.InvariantCulture. So this is inconsistency rather than an unknown pattern, and it briefly fooled my own grep into reporting it as a defect.
How the other *arr projects handle it
Readarr, the closest lineage, keeps one shared helper and pins only the path that needs it. TryParseExtensions.cs:
public static double? ParseDouble(this string source)
{
if (double.TryParse(source.Replace(',', '.'), NumberStyles.Number, CultureInfo.InvariantCulture, out var result))ParseInt32 and ParseInt64 alongside it stay bare, which matches what I found here: the integer parses are not where the damage is.
Sonarr is the closer analogue for this particular field, because it reads the same tool. It does not parse ffprobe's JSON at all. It takes Openur.FFMpegCore and Openur.FFprobeStatic as package references and asks for a TimeSpan:
var analysis = FFProbe.Analyse(filename, customArguments: "-probesize 50000000");
mediaInfoModel.RunTime = GetBestRuntime(analysis.PrimaryAudioStream?.Duration, primaryVideoStream?.Duration, analysis.Format.Duration);
The library's own helpers are named ParseDoubleInvariant and ParseIntInvariant, so the culture question is settled inside the dependency rather than at each call site.
And when Sonarr did hand-parse a number out of a media tool, the fix was exactly the change proposed here. bd601332, 2014, "Framerate in mediainfo is now parsed culture invariant":
-Decimal.TryParse(mediaInfo.Get(StreamKind.Video, 0, "FrameRate"), out videoFrameRate);
+Decimal.TryParse(mediaInfo.Get(StreamKind.Video, 0, "FrameRate"), NumberStyles.AllowDecimalPoint, CultureInfo.InvariantCulture, out videoFrameRate);
Worth being accurate about how the convention is held up over there: no analyzer enforces it. CA1305 is set to suggestion in those repos and there is no written policy. It is call-site habit plus tests parameterized over several cultures.
Options
Smallest: pass NumberStyles and CultureInfo.InvariantCulture at the six sites above. Six one-line changes, no new dependency.
Readarr's shape: add the shared ParseDouble helper and route the numeric parses through it, so the next call site inherits the right behaviour instead of having to remember.
Sonarr's shape, for the duration field specifically: stop hand-parsing ffprobe JSON and take a typed reader. That is a real dependency decision rather than a bug fix, and it has a bearing on #791, so I mention it as context rather than as a proposal here.
Turning CA1305 on is the other half of any of these, and it will light up the DateTime and int sites too, which is why I would not turn it on in the same change as a fix.
What I can do
The ordering fix from #795 is written and tested over invariant, en-US, de-DE and fr-FR. The six sites here are measured but not written, and I would rather agree the shape before writing them than send a PR that picks one for you. Tell me which you want and I will open it, or if you would rather do this inside #717's area yourself, say so and I will leave it alone.
#795 is one instance of a wider pattern. The worst instance is duration.
Duration
FfprobeMetadataMapperparsesformat.durationout of ffprobe's JSON with no format provider:Listenarr/listenarr.infrastructure/Ffmpeg/Metadata/FfprobeMetadataMapper.cs
Lines 48 to 52 in 4555ad2
ffprobe emits that field as a string and always uses
., regardless of the locale it runs under. I checked rather than assumed, in a container withde_DE.UTF-8actually generated andawkprinting1,50alongside it to prove the locale was live:The value does not change. What the parse makes of it does:
3600.53600543200.25043200250.is the group separator in de-DE, so the decimal point is silently eaten and a twelve hour book is recorded as 43,200,250 seconds. In fr-FR the parse fails and the duration is lost. That value is persisted: it becomesDurationSecondson the file record throughLibraryManualScanWorkflow,AudiobookFileService,MetadataRescanServiceandAdminMetadataController.Who this affects
Same precondition as #795, and worth stating before the list below. A stock container runs under the invariant culture and is unaffected. It takes a real culture reaching the process, which happens when
LANGorLC_ALLis set, and on a desktop install that inherits the OS locale. I have not tested the Windows or macOS builds and am not claiming anything about them.The other sites
Sizes, all parsed from strings that external services emit with
.:MyAnonamouseSizeParser.cs:36and:49SabnzbdResponseMapper.cs:201and:320TorznabNewznabValueParser.cs:40, which regexes1.5out of"1.5 GB"and then bare-parses itUnder de-DE a 1.5 GB release is read as 15 GB; under fr-FR the parse fails and the size is zero.
Two more are already covered by existing work:
Audiobook.cs:108andDownloadImportService.cs:371are what #763 fixes, andAudibleSeriesWorkflow.cs:342is #795.Counting only
decimal,doubleandDateTime, and excluding tests and mocks, canary has 27 parses with no format provider and 5 that pass one. I am deliberately not claiming theDateTimeones are broken: ISO 8601 and RFC822 both parse correctly under every culture I tried, and the only failure I could produce needed an ambiguous numeric date, where the right answer depends on what the source emits.Already correct in this codebase
NzbgetHistoryReader.cs:157parses megabytes withNumberStyles.Float, CultureInfo.InvariantCulture. So this is inconsistency rather than an unknown pattern, and it briefly fooled my own grep into reporting it as a defect.How the other *arr projects handle it
Readarr, the closest lineage, keeps one shared helper and pins only the path that needs it.
TryParseExtensions.cs:ParseInt32andParseInt64alongside it stay bare, which matches what I found here: the integer parses are not where the damage is.Sonarr is the closer analogue for this particular field, because it reads the same tool. It does not parse ffprobe's JSON at all. It takes
Openur.FFMpegCoreandOpenur.FFprobeStaticas package references and asks for aTimeSpan:The library's own helpers are named
ParseDoubleInvariantandParseIntInvariant, so the culture question is settled inside the dependency rather than at each call site.And when Sonarr did hand-parse a number out of a media tool, the fix was exactly the change proposed here.
bd601332, 2014, "Framerate in mediainfo is now parsed culture invariant":Worth being accurate about how the convention is held up over there: no analyzer enforces it. CA1305 is set to
suggestionin those repos and there is no written policy. It is call-site habit plus tests parameterized over several cultures.Options
Smallest: pass
NumberStylesandCultureInfo.InvariantCultureat the six sites above. Six one-line changes, no new dependency.Readarr's shape: add the shared
ParseDoublehelper and route the numeric parses through it, so the next call site inherits the right behaviour instead of having to remember.Sonarr's shape, for the duration field specifically: stop hand-parsing ffprobe JSON and take a typed reader. That is a real dependency decision rather than a bug fix, and it has a bearing on #791, so I mention it as context rather than as a proposal here.
Turning CA1305 on is the other half of any of these, and it will light up the
DateTimeandintsites too, which is why I would not turn it on in the same change as a fix.What I can do
The ordering fix from #795 is written and tested over invariant, en-US, de-DE and fr-FR. The six sites here are measured but not written, and I would rather agree the shape before writing them than send a PR that picks one for you. Tell me which you want and I will open it, or if you would rather do this inside #717's area yourself, say so and I will leave it alone.