| Message ID | 20221005091032.3014-3-br015@umbiko.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | User-visible Config.in feature sub-options | expand |
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? > + This is mandatory for the sticker database. Not sure what the sticker database is :-) > > 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 ? > > 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? If you want to play Ogg/Vorbis files, it's quite obvious that the "vorbis" option needs to be enabled. > +# opus needs to be encapsulated in a container format, here ogg Yes, and? > 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? 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. Thanks! Thomas
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
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. + This is mandatory for the sticker database. 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. 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 + comment "Input plugins" config BR2_PACKAGE_MPD_CDIO_PARANOIA diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk index 4d73e6de03..feab894f0f 100644 --- a/package/mpd/mpd.mk +++ b/package/mpd/mpd.mk @@ -230,6 +230,7 @@ else MPD_CONF_OPTS += -Dopenal=disabled endif +# opus needs to be encapsulated in a container format, here ogg 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 ifeq ($(BR2_PACKAGE_MPD_VORBIS),y) MPD_DEPENDENCIES += libvorbis MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled
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(-)