GNOME Bugzilla – Bug 776413
flacenc: Wrong quality levels in GStreamer, level 9 doesn't exist in libflac
Last modified: 2018-11-03 15:15:19 UTC
According to libflac [1], there are 9 compression levels, 0…8. GstFlacEncQuality (values for the "quality" property in GstFlacEnc) has 10 compression levels, 0…9. When trying to encode to FLAC through API with quality set to 9, I'm getting this GError: GLib.Error: gst-library-error-quark: Die Unterstützungsbibliothek konnte nicht initialisiert werden. (3) plus this debug message: gstflacenc.c(872): gst_flac_enc_set_format (): /GstPipeline:pipeline0/GstFlacEnc:flacenc0: could not initialize encoder (wrong parameters?) 11 Steps to reproduce: 1. run: $ gst-launch-1.0 filesrc location=test.mp3 ! mpegaudioparse ! mad ! audioconvert ! flacenc quality=8 ! filesink location=test.flac" which runs fine. 2. run $ gst-launch-1.0 filesrc location=test.mp3 ! mpegaudioparse ! mad ! audioconvert ! flacenc quality=9 ! filesink location=test.flac" gives an error message instead: Setting pipeline to PAUSED ... Pipeline is PREROLLING ... ERROR: from element /GstPipeline:pipeline0/GstFlacEnc:flacenc0: Could not initialize supporting library. Additional debug info: gstflacenc.c(872): gst_flac_enc_set_format (): /GstPipeline:pipeline0/GstFlacEnc:flacenc0: could not initialize encoder (wrong parameters?) 11 ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ... I'm running gstreamer1-plugins-good-1.10.2-1.fc25.x86_64 on Fedora 25. [1] https://www.xiph.org/flac/api/group__flac__stream__encoder.html#gae49cf32f5256cb47eecd33779493ac85
Do you want to provide a patch for this?
(In reply to Sebastian Dröge (slomo) from comment #1) > Do you want to provide a patch for this? I'd prefer not to, since I don't have any clue how you want to handle an API/ABI break like this.
If it never worked, it's not really an API/ABI break :) I'm just wondering if this is something that changed in libflac at some point or if we really had this wrong for almost 10 years without anyone noticing
(In reply to Tim-Philipp Müller from comment #3) > If it never worked, it's not really an API/ABI break :) Ok, that's right. > I'm just wondering if this is something that changed in libflac at some > point or if we really had this wrong for almost 10 years without anyone > noticing I was wondering the same and had a look at libflac changelogs which didn't note any changes in compression levels. Anyway, libFLAC specifies > A value larger than 8 will be treated as 8. in https://git.xiph.org/?p=flac.git;f=include/FLAC/stream_encoder.h;hb=HEAD#l811 so GStreamer is doing something differently weird. It seems that GstFlacEncParams flacenc_params[] in https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/ext/flac/gstflacenc.c#n185 differs from Xiph's CompressionLevels compression_levels[] https://git.xiph.org/?p=flac.git;f=src/libFLAC/stream_encoder.c;hb=HEAD#l102 shouldn't they be the same? Should we request the CompressionLevels struct to be public? For example: At compression level 7, GStreamer sets max_lpc_order=8 whereas libFLAC sets it to 12. Also, FLAC__bool do_exhaustive_model_search is always False, whereas the GStreamer implementation sets it to TRUE for level 7 to 9. Other parameters differ too. It looks like the author of GstFlacEnc just invented compression level 9, at least that is what the parameters look like.
It would be good if that table could be made public indeed, in one way or another. But for now we should probably update it with what is in latest libFLAC. Do you want to provide a patch for that?
Created attachment 344486 [details] [review] Fix number of quality levels A minimal patch for both stable and master branch. More to come.
The "From" (i.e. Author) in your patch seems corrupted, that's just random letters. But didn't you say that there's also a mismatch between settings for our other quality levels?
(In reply to Sebastian Dröge (slomo) from comment #7) > The "From" (i.e. Author) in your patch seems corrupted, that's just random > letters. Interesting. Haven't seen that before. Looks like another bug in gitg or libgit. I've reported it at https://bugzilla.gnome.org/show_bug.cgi?id=777932
(In reply to Sebastian Dröge (slomo) from comment #7) > But didn't you say that there's also a mismatch between settings for our > other quality levels? Yes, I did. Most notably, the FLAC__stream_encoder_set_compression_level does NOT set the blocksize any more, but sets apodization. Is it Ok if I 1. introduce a new PROP_APODIZATION value to the enum and a new apodization parameter? 2. and reorder enum entries? 3. change GstFlacEncParams: remove blocksize, add apodization, reorder fields 4. change gst_flac_enc_update_quality to use FLAC__stream_encoder_set_compression_level instead of relying on the flacenc_params array and get rid of flacenc_params array, leaving only default values of each parameter. Still, I need to dive into GLib a bit to provide the patch, because I do not understand enough of how GParam properties works.
Created attachment 344529 [details] [review] Fix number of quality levels (with fixed From: field)
(In reply to Christian Stadelmann from comment #9) > (In reply to Sebastian Dröge (slomo) from comment #7) > > But didn't you say that there's also a mismatch between settings for our > > other quality levels? > > Yes, I did. Most notably, the FLAC__stream_encoder_set_compression_level > does NOT set the blocksize any more, but sets apodization. > Is it Ok if I > 1. introduce a new PROP_APODIZATION value to the enum and a new apodization > parameter? > 2. and reorder enum entries? Reorder which enum entries? > 3. change GstFlacEncParams: remove blocksize, add apodization, reorder fields > 4. change gst_flac_enc_update_quality to use > FLAC__stream_encoder_set_compression_level instead of relying on the > flacenc_params array and get rid of flacenc_params array, leaving only > default values of each parameter. Sounds like a plan
Is there any simple way to test my code? I am used to gst-launch-1.0, but how do I get it to use the newly compiled flac library? Why do you `#define` constants instead of making them `const`? Would you mind if I changed this to using `const`? (In reply to Sebastian Dröge (slomo) from comment #11) > (In reply to Christian Stadelmann from comment #9) > > Is it Ok if I > > 1. introduce a new PROP_APODIZATION value to the enum and a new apodization > > parameter? > > 2. and reorder enum entries? > > Reorder which enum entries? The ones in the anonymous enum starting around line 106.
(In reply to Christian Stadelmann from comment #12) > Is there any simple way to test my code? I am used to gst-launch-1.0, but > how do I get it to use the newly compiled flac library? You mean the new libgstflac.so? Either overwrite the existing one from your system, use something like gst-uninstalled or just set GST_PLUGIN_PATH to the location of the new .so. It should override the system path then. > Why do you `#define` constants instead of making them `const`? Would you > mind if I changed this to using `const`? Convention > (In reply to Sebastian Dröge (slomo) from comment #11) > > (In reply to Christian Stadelmann from comment #9) > > > Is it Ok if I > > > 1. introduce a new PROP_APODIZATION value to the enum and a new apodization > > > parameter? > > > 2. and reorder enum entries? > > > > Reorder which enum entries? > > The ones in the anonymous enum starting around line 106. You mean the PROP_* ones? Why would you like to reorder them? I think it's ok to do so, but doesn't seem very useful other than making the diff bigger and increasing the risk of merge conflicts :)
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/335.