| Message ID | 1419554775-11560-2-git-send-email-bos@je-eigen-domein.nl |
|---|---|
| State | Superseded |
| Headers | show |
Dear Floris Bos, On Fri, 26 Dec 2014 01:46:15 +0100, Floris Bos wrote: > +if BR2_PACKAGE_LIBVNCSERVER > + > +config BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG > + bool "TightPNG encoding support" > + select BR2_PACKAGE_JPEG > + select BR2_PACKAGE_LIBPNG > + help > + TightPNG encoding speeds up HTML5 based VNC clients like noVNC. > + > + http://wiki.qemu.org/VNC_Tight_PNG I've tried to read a bit, but I don't really understand whether JPEG is absolutely necessary to get TightPNG support. The webpage you mention says "If the client indicates JPEG support by sending a JPEG quality pseudo-encoding, then the server can send JPEG, because PNG will only replace the basic compression type used in normal tight.", which seems to indicate that the JPEG support is only optional. > +endif > diff --git a/package/libvncserver/libvncserver.mk b/package/libvncserver/libvncserver.mk > index b26d5b9..8479a62 100644 > --- a/package/libvncserver/libvncserver.mk > +++ b/package/libvncserver/libvncserver.mk > @@ -56,4 +56,10 @@ else > LIBVNCSERVER_CONF_OPTS += --without-zlib > endif > > +ifeq ($(BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG),y) > +LIBVNCSERVER_DEPENDENCIES += libpng So we don't really need JPEG support after all, since you don't depend on it? > +else > +LIBVNCSERVER_CONF_OPTS += --without-png > +endif > + > $(eval $(autotools-package)) Thanks, Thomas
On 26.12.2014 18:08, Thomas Petazzoni wrote: > On Fri, 26 Dec 2014 01:46:15 +0100, Floris Bos wrote: > >> +if BR2_PACKAGE_LIBVNCSERVER >> + >> +config BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG >> + bool "TightPNG encoding support" >> + select BR2_PACKAGE_JPEG >> + select BR2_PACKAGE_LIBPNG >> + help >> + TightPNG encoding speeds up HTML5 based VNC clients like noVNC. >> + >> + http://wiki.qemu.org/VNC_Tight_PNG > > I've tried to read a bit, but I don't really understand whether JPEG is > absolutely necessary to get TightPNG support. The webpage you mention > says "If the client indicates JPEG support by sending a JPEG > quality pseudo-encoding, then the server can send JPEG, because PNG > will only replace the basic compression type used in normal tight.", > which seems to indicate that the JPEG support is only optional. JPEG and PNG are both necesarry. Libvncserver will only compile tight.c if HAVE_LIBJPEG https://github.com/LibVNC/libvncserver/blob/master/libvncserver/Makefile.am#L49 And inside tight.c png is optional, jpeg is not. > >> +endif >> diff --git a/package/libvncserver/libvncserver.mk >> b/package/libvncserver/libvncserver.mk >> index b26d5b9..8479a62 100644 >> --- a/package/libvncserver/libvncserver.mk >> +++ b/package/libvncserver/libvncserver.mk >> @@ -56,4 +56,10 @@ else >> LIBVNCSERVER_CONF_OPTS += --without-zlib >> endif >> >> +ifeq ($(BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG),y) >> +LIBVNCSERVER_DEPENDENCIES += libpng > > So we don't really need JPEG support after all, since you don't depend > on it? JPEG is added by the existing code: == ifeq ($(BR2_PACKAGE_JPEG),y) LIBVNCSERVER_DEPENDENCIES += jpeg else LIBVNCSERVER_CONF_OPTS += --without-jpeg endif == Wasn't sure whether to touch that, was afraid that would change behavior for existing users. Your sincerely, Floris Bos
Dear Floris Bos, On Fri, 26 Dec 2014 18:37:15 +0100, Floris Bos wrote: > JPEG and PNG are both necesarry. > > Libvncserver will only compile tight.c if HAVE_LIBJPEG > > https://github.com/LibVNC/libvncserver/blob/master/libvncserver/Makefile.am#L49 > > And inside tight.c png is optional, jpeg is not. Ok, so I believe we should rather do: +config BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG + bool "TightPNG encoding support" + select BR2_PACKAGE_JPEG + select BR2_PACKAGE_LIBPNG + help + TightPNG encoding speeds up HTML5 based VNC clients like noVNC. + + http://wiki.qemu.org/VNC_Tight_PNG in Config.in (i.e as you did) And then in libvncserver.mk, do: ifeq ($(BR2_PACKAGE_LIBPNG),y) LIBVNCSERVER_DEPENDENCIES += libpng else LIBVNCSERVER_CONF_OPTS += --without-png endif This way, people not using tightpng support but having libpng enabled will have PNG support in libvncserver. Can you rework your patch in this direction? Thanks, Thomas
On 12/27/2014 02:51 PM, Thomas Petazzoni wrote: > Dear Floris Bos, > > On Fri, 26 Dec 2014 18:37:15 +0100, Floris Bos wrote: > >> JPEG and PNG are both necesarry. >> >> Libvncserver will only compile tight.c if HAVE_LIBJPEG >> >> https://github.com/LibVNC/libvncserver/blob/master/libvncserver/Makefile.am#L49 >> >> And inside tight.c png is optional, jpeg is not. > Ok, so I believe we should rather do: > > +config BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG > + bool "TightPNG encoding support" > + select BR2_PACKAGE_JPEG > + select BR2_PACKAGE_LIBPNG > + help > + TightPNG encoding speeds up HTML5 based VNC clients like noVNC. > + > + http://wiki.qemu.org/VNC_Tight_PNG > > in Config.in (i.e as you did) > > And then in libvncserver.mk, do: > > ifeq ($(BR2_PACKAGE_LIBPNG),y) > LIBVNCSERVER_DEPENDENCIES += libpng > else > LIBVNCSERVER_CONF_OPTS += --without-png > endif > > This way, people not using tightpng support but having libpng enabled > will have PNG support in libvncserver. Do note that the only thing using PNG inside libvncserver is the TightPNG encoding. Think it is a bit counter-inductive to give users TightPNG support, just because another package selected libpng, even though they chose not to select the TightPNG feature. (Just like I believe it is counter-inductive that users are expected to know which dependencies are needed to get a specific feature they want, as seems to be the current case with many buildroot packages, which do not offer any feature options).
Dear Floris Bos, On Sat, 27 Dec 2014 18:43:40 +0100, Floris Bos wrote: > Do note that the only thing using PNG inside libvncserver is the > TightPNG encoding. > Think it is a bit counter-inductive to give users TightPNG support, just > because another package selected libpng, even though they chose not to > select the TightPNG feature. > > (Just like I believe it is counter-inductive that users are expected to > know which dependencies are needed to get a specific feature they want, > as seems to be the current case with many buildroot packages, which do > not offer any feature options). This topic has been the source of many discussions in the Buildroot community, and we're trying to find a balance between two options: * Have sub-options in each package for all the various possible optional features they have. This makes it more explicit for users, but on the flip side generates a huge maintenance burden, as we would have a much much larger number of Config.in options to maintain. * Make optional features automatically enabled. This is less explicit for users, but it is sometimes good (like if you have OpenSSL enabled, enable SSL support everywhere it is possible), and avoids creating zillions of Config.in options. So generally, on a case by case basis, we decide which of the solution is the most appropriate. Sometimes the latter is good enough and avoids creating more Config.in options, sometimes the former is really necessary to make it explicit to the user what is happening. I don't think it is possible to settle on one or the other solution and apply this decision unconditionally to all packages. Best regards, Thomas
diff --git a/package/libvncserver/Config.in b/package/libvncserver/Config.in index 07b77f5..7d8272f 100644 --- a/package/libvncserver/Config.in +++ b/package/libvncserver/Config.in @@ -5,3 +5,16 @@ config BR2_PACKAGE_LIBVNCSERVER libvncserver is a VNC server/client library. http://libvncserver.sourceforge.net/ + +if BR2_PACKAGE_LIBVNCSERVER + +config BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG + bool "TightPNG encoding support" + select BR2_PACKAGE_JPEG + select BR2_PACKAGE_LIBPNG + help + TightPNG encoding speeds up HTML5 based VNC clients like noVNC. + + http://wiki.qemu.org/VNC_Tight_PNG + +endif diff --git a/package/libvncserver/libvncserver.mk b/package/libvncserver/libvncserver.mk index b26d5b9..8479a62 100644 --- a/package/libvncserver/libvncserver.mk +++ b/package/libvncserver/libvncserver.mk @@ -56,4 +56,10 @@ else LIBVNCSERVER_CONF_OPTS += --without-zlib endif +ifeq ($(BR2_PACKAGE_LIBVNCSERVER_TIGHTPNG),y) +LIBVNCSERVER_DEPENDENCIES += libpng +else +LIBVNCSERVER_CONF_OPTS += --without-png +endif + $(eval $(autotools-package))
Signed-off-by: Floris Bos <bos@je-eigen-domein.nl> --- package/libvncserver/Config.in | 13 +++++++++++++ package/libvncserver/libvncserver.mk | 6 ++++++ 2 files changed, 19 insertions(+)