Can't load any files with gstreamer 1.19.2: "The specified file is not supported!: Either because it is broken or not an audio file."

[Adam Williamson]

I just went to downsample a few high-res FLAC files and found that soundconverter can't load them. In fact, it can't load any audio files I've tried, including ones I successfully converted recently.

I'm on Fedora 35. soundconverter version is soundconverter-4.0.1-3.fc35.noarch. I last used soundconverter successfully on September 24th. Soon after that, I ran a system update that installed 221 updates. I'll include the list separately, but soundconverter itself isn't in it; several Python packages are, but the most likely culprit seems to be:

    Upgrade gstreamer1-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-plugins-bad-free-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-plugins-base-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-plugins-good-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-plugins-good-gtk-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-plugins-good-qt-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-plugins-ugly-free-1.19.2-1.fc35.x86_64 @updates-testing
    Upgrade gstreamer1-vaapi-1.19.2-1.fc35.x86_64 @updates-testing

It was upgraded from 1.19.1 to 1.19.2. I haven't done any other updates since. Now, whenever I try to add any file - including the same files I successfully converted on September 24th - I get this error:

"The specified file is not supported!: Either because it is broken or not an audio file."

Running from a console with -d doesn't add much detail:

[adamw@xps13k audio]$ soundconverter -d
INFO: soundconverter, line 237, soundconverter 4.0.1
INFO: ui.py, line 273, analysing file integrity
INFO: ui.py, line 293, adding: 1 files
INFO: ui.py, line 309, Discovered 1 audiofiles in 0.3 s
ERROR: ui.py, line 120, The specified file is not supported!: Either because it is broken or not an audio file.
DEBUG: ui.py, line 402, Added 1 files in 1.80s (scan 0.07s, add 1.73s)

[Adam Williamson (awilliamson)]

It looks like `_analyze_file` is returning here:

            taglist = info.get_tags()
            if not taglist: return

because I added a couple of debug log lines around it, and the one from before that line triggers, but the one from after it doesn't. So we're getting nothing in `taglist` for some reason.

[sezanzeb (sezanzeb)]

Hi

does adding `logger.debug(str(error))` below the except yield anything interesting?

did you add the `if not taglist: return` line yourself? What happens if you comment the lines with info.get_tags and taglist.foreach?

gstreamer 1.19 is not available in the manjaro repos yet, so I can't try to reproduce it right now unfortunately.

[Adam Williamson (awilliamson)]

Aha, I suspect this may be caused by:

https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/1275

that landed between 1.19.1 and 1.19.2 upstream, and seems in the general area.

[sezanzeb (sezanzeb)]

I added the logger.debug to my fork on the "gstreamer-1.19-bug" branch for your convenience, you can try to install that if you want: https://github.com/sezanzeb/soundconverter/tree/gstreamer-1.19-bug

[Adam Williamson (awilliamson)]

"did you add the `if not taglist: return` line yourself?"

No. That's soundconverter's own code, and it's where we're bailing out of discovery, because we're not finding any tags - the code expects that we will.

"does adding `logger.debug(str(error))` below the except yield anything interesting?"

I don't think it would do anything, because there's no exception. We're not hitting the except block. We're just hitting the "no taglist" condition and returning nothing. That means `sound_file.readable` is still `False`, because we don't reach the line where it's set to `True`, and so `ui.py` considers the file unreadable.

[Adam Williamson (awilliamson)]

So this change "works", though it's making a kinda shoddy assumption (that there'll be just one audio stream in the file, or at least that the first audio stream in the file has the tags we want):

--- a/soundconverter/gstreamer/discoverer.py
+++ b/soundconverter/gstreamer/discoverer.py
@@ -123,7 +123,7 @@ class DiscovererThread(Thread):
             # whatever anybody might ever need from it, here it is:
             sound_file.info = info

- taglist = info.get_tags()
+ taglist = info.get_audio_streams()[0].get_tags()
             if not taglist: return
             taglist.foreach(lambda *args: self._add_tag(*args, sound_file))

[Adam Williamson (awilliamson)]

Just in case you didn't notice it, there's some discussion on the upstream MR I linked before where the upstream author acknowledged the problem but is worried there's no way to fix it without breaking the other cases. I suppose one interesting question would be how this apparently worked for both soundconverter and the other affected apps before 1.16. Maybe I'll go back and check how things worked on both sides in 1.14.

[sezanzeb (sezanzeb)]

> No. That's soundconverter's own code

That return is not in the current py3k branch. I then realized this is in the 4.0.1 tag and was added 5 months ago, but only there. @kassoulet?

https://github.com/kassoulet/soundconverter/blob/4.0.1/soundconverter/gstreamer/discoverer.py
https://github.com/kassoulet/soundconverter/blob/py3k/soundconverter/gstreamer/discoverer.py

[sezanzeb (sezanzeb)]

How about something like https://github.com/kassoulet/soundconverter/pull/55

[GautierPortet (kassoulet)]

Yep, I'm the one that inserted the bail off in this function, because I was annoyed by the error messages when reading files without tags. Well I seems I forgot to actually push the change.

I'll pull the correction, with one change: iterate on all audio streams instead of just the first.

Thank you both for the analysis and correction!

[Adam Williamson (awilliamson)]

@sezanzeb I think I might want to make the line a bit 'safer' for the case where the file isn't actually an audio file, or whatever; things will still work since we're in a `try` block that catches every exception, but it'd probably be a bit clearer what's happening if we don't rely on that. I'll post a suggestion tomorrow.

Indeed the commit is only on the 4.x branch (whatever branch that is...), but I don't think this means things would be fine on the dev branch; surely the lack of the expected tags is going to cause problems later?

[Adam Williamson (awilliamson)]

@kassoulet yeah, what I was actually planning to do tomorrow was more or less: wrap that whole little bit in a `for taglist in [global taglist plus all audio stream taglists]`: , and just iterate over *all* those taglists, running `self._add_tag` on every tag from each of those taglists. WDYT?

[GautierPortet (kassoulet)]

I think this is the right way to handle it, what do you think ? :
https://github.com/kassoulet/soundconverter/commit/99b0b011282b71f0a35f2a798d6100c7868a194b

[sezanzeb (sezanzeb)]

Looks good 👍

[Adam Williamson (awilliamson)]

Sure, not quite the same structure I had thought of but does the same thing. Should we replace the old `if not taglist: return` line with `if not sound_file.tags: return` or similar after that's done?

[sezanzeb (sezanzeb)]

This change is already in the latest py3k source on github. I think this issue is probably solved and doesn't require further changes, tests are passing as well

[Adam Williamson (awilliamson)]

Yeah, I backported the fix to Fedora and it works. I'd just wonder if we want a check whether there are any tags after we read them all (to avoid any avoidable errors if a non-audio file is added), and maybe if we should add the `if taglist` check to the `for audio_stream in info.get_audio_streams():` block too, just to be super safe - theoretically, if we somehow get a null taglist from `audio_stream.get_tags()` there, we'd hit an exception trying to call `taglist.foreach` on it and fail to parse the file, *even if we already got valid tags from the global info*.

[sezanzeb (sezanzeb)]

> if we want a check whether there are any tags after we read them all (to avoid any avoidable errors if a non-audio file is added)

Files can't be classified as non-audio by looking at tags, which might be empty

> and maybe if we should add the `if taglist` check

```
            for audio_stream in info.get_audio_streams():
                # Read tags for each audio stream
                taglist = audio_stream.get_tags()
                if taglist:
                    taglist.foreach(lambda *args: self._add_tag(*args, sound_file))
```

that probably makes sense yes

[Adam Williamson (awilliamson)]

OK, so if I read that correct, you're saying the check that was added on the 4.0.1 branch - `if not taglist: return` - was actually *wrong*? We should continue trying to handle such a file as it may be a valid audio file with no tags, and we can still do something useful with those?

If so, I agree it doesn't make sense to replace that check, then :)

[GautierPortet (kassoulet)]

Yes, the test was actually wrong, as we can convert the file even the tags were not read. I'll add a second test and release a new version to fix this issue.