Re: [Buildroot] [PATCH v2 2/4] package/mpd: add/enhance (kconfig + code) comments

[Andreas Ziegler <br015@umbiko.net>]

Hi Thomas, *,

Thank you for picking this up.

On 2023-08-06 19:45, Thomas Petazzoni wrote:
> Hello Andreas,
> 
> On Wed,  5 Oct 2022 11:10:30 +0200
> Andreas Ziegler <br015@umbiko.net> wrote:
> 
>> Align kconfig comments with descriptions in the mpd manual.
>> Add a kconfig comment to highlight the impact of ogg/vorbis selection.
>> Add comments to makefile to explain remaining multiple dependency 
>> creation.
>> 
>> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
>> ---
>> Changes v1 -> v2:
>>  - make this a separate patch
>> 
>>  package/mpd/Config.in | 9 ++++++---
>>  package/mpd/mpd.mk    | 2 ++
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/package/mpd/Config.in b/package/mpd/Config.in
>> index 8f0af7b2d3..2606008e90 100644
>> --- a/package/mpd/Config.in
>> +++ b/package/mpd/Config.in
>> @@ -33,7 +33,7 @@ config BR2_PACKAGE_MPD_SQLITE
>>  	select BR2_PACKAGE_SQLITE
>>  	help
>>  	  Enable sqlite database support.
>> -	  If you don't use sqlite it will use an ASCII database.
> 
> Why is this removed?

It raises false hopes. Mpd uses an in-memory database that is persisted 
in a proprietary file format. The format may change between releases and 
there is no way to use a SQL database for storage. All that the 
sqlite=enabled feature does is enable the sticker database.

> 
>> +	  This is mandatory for the sticker database.
> 
> Not sure what the sticker database is :-)

I will add more information from the mpd manual:
+	  Support for the sticker database.  “Stickers” are pieces of
+	  information attached to songs. Some clients use them to store
+	  ratings and other volatile data.

> 
>> 
>>  config BR2_PACKAGE_MPD_ZZIP
>>  	bool "zzip"
>> @@ -81,8 +81,8 @@ comment "Decoder plugins"
>>  config BR2_PACKAGE_MPD_DSD
>>  	bool "dsd"
>>  	help
>> -	  Enable Digital Speech Decoder (DSD) support to play audio
>> -	  files encoded in a digital speech format.
>> +	  Direct Stream Digital (DSD) support to play audio
>> +	  files encoded in single bit format.
> 
> Is this change really relevant ?
> 

The Digital Speech [1] format exists, but this is not the format 
referred to here. MPD plays SA-CD rips [2]

Maybe:
+	  Direct Stream Digital (DSD) support to play delta-sigma
+	  encoded files in formats DSDIFF and DSF.
+
+	  This is the sample format used on Super Audio CDs.

>> 
>>  config BR2_PACKAGE_MPD_FAAD2
>>  	bool "faad2"
>> @@ -210,6 +210,9 @@ config BR2_PACKAGE_MPD_TWOLAME
>>  	help
>>  	  Enable TwoLAME mp2 encoding.
>> 
>> +comment "for ogg/vorbis encoding enable vorbis decoder"
>> +	depends on !BR2_PACKAGE_MPD_VORBIS
> 
> I don't understand why this comment is needed. We usually don't put
> comments about all dependencies on other packages. Why for this one?

Options are structured by function. Since ogg/vorbis codecs must be 
selected simultaneously (according to the Buildroot standard), a feature 
that is located under *decoding* must be enabled for *encoding*. Breaks 
up the UI a little bit, therefore a comment.

> If you want to play Ogg/Vorbis files, it's quite obvious that the
> "vorbis" option needs to be enabled.

This is a different use case. The encoder is used for streaming 
protocols, similar to internet radio stations.

>> +# opus needs to be encapsulated in a container format, here ogg
> 
> Yes, and?

Most mpd options have exactly one dependency. Feature name and library 
name match; easy to see that everything is as it should be. So I added 
comments whenever there was a deviation, just to show that this is not 
an error, but intended.

>>  ifeq ($(BR2_PACKAGE_MPD_OPUS),y)
>>  MPD_DEPENDENCIES += opus libogg
>>  MPD_CONF_OPTS += -Dopus=enabled
>> @@ -317,6 +318,7 @@ else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
>>  MPD_CONF_OPTS += -Dupnp=disabled
>>  endif
>> 
>> +# handle decoder and encoder simultaneously
> 
> Yes, and?
> 

Again, a deviation from the one feature /one selection pattern.

> Sorry, but I don't really understand the value of the changes proposed
> in this commit. I'll mark it as Changes Requested for the time being.
> Feel free to resubmit with more details if needed. Also, look at other
> Buildroot packages, we try to do things consistently, so doing
> something "special" in MPD is unlikely to be accepted.

Some packages - for example mpd - are rather difficult to configure 
correctly. So why not add a bit more information, especially in the user 
interface ;-)

I expect your comments and will follow up with a new version.

Kind regards,
Andreas

> Thanks!
> 
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com

[1] https://en.wikipedia.org/wiki/Digital_Speech_Standard
[2] https://en.wikipedia.org/wiki/Direct_Stream_Digital
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot