Bug 823096 - Review Request: nvidia-texture-tools - Collection of image processing and texture manipulation tools
Review Request: nvidia-texture-tools - Collection of image processing and tex...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-18 23:12 EDT by Paulo Andrade
Modified: 2013-01-11 19:02 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-29 01:26:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tcallawa: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paulo Andrade 2012-05-18 23:12:12 EDT
Spec URL: http://fedorapeople.org/~pcpa/nvidia-texture-tools.spec
SRPM URL: http://fedorapeople.org/~pcpa/nvidia-texture-tools-2.0.8-1.fc18.src.rpm
Description: 
The NVIDIA Texture Tools is a collection of image processing and texture
manipulation tools, designed to be integrated in game tools and asset
conditioning pipelines.

The primary features of the library are mipmap and normal map generation,
format conversion and DXT compression.

DXT compression is based on Simon Brown's squish library. The library also
contains an alternative GPU-accelerated compressor that uses CUDA and is
one order of magnitude faster.
Comment 1 Paulo Andrade 2012-05-18 23:15:38 EDT
koji --scratch build pass on i686 and x86_64.

Patches were reported upstream at
http://code.google.com/p/nvidia-texture-tools/issues/detail?id=175

This package will be required to build 0ad (#818401), as I am
reworking it to not build an internal copy of an older version
of nvidia-texture-tools.
Comment 2 Peter Lemenkov 2012-05-19 00:23:13 EDT
Lifting FE-LEGAL due to possible issues with S3TC

http://code.google.com/p/nvidia-texture-tools/wiki/FAQ
Comment 3 Paulo Andrade 2012-05-19 00:51:00 EDT
Thanks for noticing it.

A grep -ri s3tc shows only references to it in
src/nvtt/squish/texture_compression_s3tc.txt

otherwise, the only other reference is in
src/nvtt/CompressDXT.cpp

#if defined(HAVE_S3QUANT)
#include "s3tc/s3_quant.h"
#endif

and HAVE_S3QUANT is not defined.

and the actual code, in the same file is also
inside an inclusion guard:

#if defined(HAVE_S3QUANT)
void nv::s3CompressDXT1(const Image * image, const nvtt::OutputOptions::Private & outputOptions)
{
...
}
#endif

Actually, that same source also has another

#if defined(HAVE_ATITC)
#include "atitc/ATI_Compress.h"
#endif

and a void nv::atiCompressDXT1 function compiled
if HAVE_ATITC is defined.

So, from my understanding, there is no binary code
using it, neither exposing interfaces to it.

But there is source code that is not compiled,
that may interface those if the devel/runtime is
detected at build time.
Comment 4 Adam Jackson 2012-05-21 10:51:28 EDT
(In reply to comment #3)
> Thanks for noticing it.
> 
> A grep -ri s3tc shows only references to it in
> src/nvtt/squish/texture_compression_s3tc.txt

Simply searching for s3tc is not enough.  There are multiple variants of S3TC, named DXT[1-5].  Which I definitely see code for in here.  I don't think this is something Fedora can ship.
Comment 5 Paulo Andrade 2012-05-21 14:09:21 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > Thanks for noticing it.
> > 
> > A grep -ri s3tc shows only references to it in
> > src/nvtt/squish/texture_compression_s3tc.txt
> 
> Simply searching for s3tc is not enough.  There are multiple variants of
> S3TC, named DXT[1-5].  Which I definitely see code for in here.  I don't
> think this is something Fedora can ship.

I was under the impression that it did only interface to
s3tc, but it actually embeds an open source implementation,
that was already blocked at
https://bugzilla.redhat.com/show_bug.cgi?id=407561

For more information, start of thread in debian,
http://lists.debian.org/debian-mentors/2010/09/msg00080.html

There is a comment about DeVIL (also in fedora)
http://lists.debian.org/debian-mentors/2010/09/msg00129.html
but from what I could read in DeVIL sources, it does not include
an implementation, and only attempts to get pointers to it
from the libGL.

And apparently debian decision was to allow it due to
not enforced patent, but that should be an option for debian
and not for Fedora/RedHat
http://lists.debian.org/debian-mentors/2010/09/msg00130.html

Nevertheless, looking more closely at 0ad build options, I see
the default is to build a bundled nvtt, or respect
--with-system-nvtt, but there is yet another --without-nvtt
that I suppose is an alternative to either textures already
compressed in 0ad-data or just not compressing it.

Reading the sources, --without-nvtt defines CONFIG2_NVTT to 0,
and only source to use it is
<0ad-src>/source/graphics/TextureConverter.cpp
with functions that can be called from
<0ad-src>/source/graphics/TextureManager.cpp

I will try a --without-nvtt build later, and see the results, e.g.
if not filling my disk, or console with logs:

-%<-
bool CTextureConverter::ConvertTexture(const CTexturePtr& texture, const VfsPath& src, const VfsPath& dest, const Settings& settings)
{
   /* load image from vfs, checks, etc */
#if CONFIG2_NVTT
        ...
    	request->compressionOptions.setQuality(m_HighQuality ? nvtt::Quality_Production : nvtt::Quality_Fastest);

	// TODO: normal maps, gamma, etc

	// Load the texture data
	request->inputOptions.setTextureLayout(nvtt::TextureType_2D, tex.w, tex.h);
	request->inputOptions.setMipmapData(tex_get_data(&tex), tex.w, tex.h);

	// NVTT copies the texture data so we can free it now
	tex_free(&tex);

	pthread_mutex_lock(&m_WorkerMutex);
	m_RequestQueue.push_back(request);
	pthread_mutex_unlock(&m_WorkerMutex);

	// Wake up the worker thread
	SDL_SemPost(m_WorkerSem);

	return true;

#else
	LOGERROR(L"Failed to convert texture \"%ls\" (NVTT not available)", src.string().c_str());
	return false;
#endif
}
-%<-


There is an upstream page about the issue at
http://trac.wildfiregames.com/wiki/CompressedTextures
Comment 6 Paulo Andrade 2012-05-21 21:33:59 EDT
I added a comment at http://trac.wildfiregames.com/ticket/1427
and tested a build with that patch and the package appears to
work. Major change was to need to disable %check in the spec
due to missing nvtt.
Comment 7 Paulo Andrade 2012-07-13 10:18:58 EDT
The package builds code under a patent and is not eligible for fedora. I made a review request for the same package in rpmfusion, but there is a workaround to build 0ad without nvtt.
Comment 8 Tom "spot" Callaway 2012-12-19 11:52:16 EST
Upon further review, this package is no longer blocked on FE-Legal. It does contain an S3TC implementation, but the patent license provided by NVIDIA has been determined to be sufficient. Please do not take the S3TC implementation from this codebase and copy it into any other code, because that would not be covered under the terms of the NVIDIA patent license (and not acceptable for Fedora).

Lifting FE-Legal
Comment 9 Tom "spot" Callaway 2012-12-19 12:02:59 EST
== Review ==

Good:

- rpmlint checks return:
nvidia-texture-tools.x86_64: W: spelling-error %description -l en_US mipmap -> mishap
nvidia-texture-tools.x86_64: W: shared-lib-calls-exit /usr/lib64/libnvcore.so.2.0.8 exit@GLIBC_2.2.5
nvidia-texture-tools.src: W: spelling-error %description -l en_US mipmap -> mishap

All safe to ignore

- package meets naming guidelines
- package meets packaging guidelines
- license (MIT) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream (6b3c83500b420ee976f61eeae16e5727e2401e133f543baeac76c66c381eed2e)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

MINOR ISSUES:

* The source url is wrong, should be:
http://nvidia-texture-tools.googlecode.com/files/%{name}-%{version}-1.tar.gz

* I don't see the value in a separate "progs" subpackage (it is only 41K). I strongly suggest just combining it into the main package (especially since the naming "nvidia-texture-tools" implies the presence of tools), but I leave that to your discretion.

Please fix the source URL (and dropping the "progs" subpackage if you choose to do so) before committing to Fedora, but this package is otherwise very well done.

APPROVED.
Comment 10 Paulo Andrade 2012-12-19 16:54:12 EST
(In reply to comment #8)
> Upon further review, this package is no longer blocked on FE-Legal. It does
> contain an S3TC implementation, but the patent license provided by NVIDIA
> has been determined to be sufficient. Please do not take the S3TC
> implementation from this codebase and copy it into any other code, because
> that would not be covered under the terms of the NVIDIA patent license (and
> not acceptable for Fedora).
> 
> Lifting FE-Legal

Many thanks for looking back at this issue and reviewing it!

I updated the package before requesting scm and should now
match fully your recommendations:

- Correct source url (#823096).
- No need for a -progs subpackage (#823096).

Spec URL: http://fedorapeople.org/~pcpa/nvidia-texture-tools.spec
SRPM URL: http://fedorapeople.org/~pcpa/nvidia-texture-tools-2.0.8-3.fc19.src.rpm
Comment 11 Paulo Andrade 2012-12-19 16:58:59 EST
New Package SCM Request
=======================
Package Name: nvidia-texture-tools
Short Description: Collection of image processing and texture manipulation tools
Owners: pcpa
Branches: f16 f17 f18
InitialCC:
Comment 12 Jon Ciesla 2012-12-20 06:51:05 EST
Git done (by process-git-requests).
Comment 13 Fedora Update System 2012-12-20 20:25:29 EST
0ad-0.0.12-2.fc16,0ad-data-0.0.12-1.fc16,nvidia-texture-tools-2.0.8-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/0ad-0.0.12-2.fc16,0ad-data-0.0.12-1.fc16,nvidia-texture-tools-2.0.8-3.fc16
Comment 14 Fedora Update System 2012-12-20 20:26:56 EST
0ad-0.0.12-2.fc17,0ad-data-0.0.12-1.fc17,nvidia-texture-tools-2.0.8-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/0ad-0.0.12-2.fc17,0ad-data-0.0.12-1.fc17,nvidia-texture-tools-2.0.8-3.fc17
Comment 15 Fedora Update System 2012-12-20 20:28:10 EST
0ad-0.0.12-2.fc18,0ad-data-0.0.12-1.fc18,nvidia-texture-tools-2.0.8-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/0ad-0.0.12-2.fc18,0ad-data-0.0.12-1.fc18,nvidia-texture-tools-2.0.8-3.fc18
Comment 16 Fedora Update System 2012-12-21 07:02:59 EST
Package 0ad-0.0.12-2.fc16, 0ad-data-0.0.12-1.fc16, nvidia-texture-tools-2.0.8-3.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing 0ad-0.0.12-2.fc16 0ad-data-0.0.12-1.fc16 nvidia-texture-tools-2.0.8-3.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-20773/0ad-0.0.12-2.fc16,0ad-data-0.0.12-1.fc16,nvidia-texture-tools-2.0.8-3.fc16
then log in and leave karma (feedback).
Comment 17 Fedora Update System 2012-12-29 01:26:21 EST
0ad-0.0.12-2.fc17, 0ad-data-0.0.12-1.fc17, nvidia-texture-tools-2.0.8-3.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 18 Fedora Update System 2012-12-29 01:27:04 EST
0ad-0.0.12-2.fc16, 0ad-data-0.0.12-1.fc16, nvidia-texture-tools-2.0.8-3.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 19 Fedora Update System 2013-01-11 19:02:11 EST
0ad-0.0.12-2.fc18, 0ad-data-0.0.12-1.fc18, nvidia-texture-tools-2.0.8-3.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.