Bug 507052

Summary: Review Request: Panini - A tool for creating perspective views from panoramic and wide angle images
Product: [Fedora] Fedora Reporter: Ankur Sinha (FranciscoD) <sanjay.ankur>
Component: Package ReviewAssignee: Nicolas Chauvet (kwizart) <kwizart>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bruno, fedora-package-review, julian.fedora, kwizart, notting, sanjay.ankur, tcallawa
Target Milestone: ---Flags: kwizart: fedora-review+
j: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.71.103-3.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-08-17 21:52:50 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Ankur Sinha (FranciscoD) 2009-06-20 08:01:13 UTC
Spec URL: http://ankursinha.fedorapeople.org/panini/panini.spec

SRPM URL: http://ankursinha.fedorapeople.org/panini/panini-0.62.83-1.fc11.src.rpm

Description: 
Panini is a visual tool for creating perspective views from panoramic
and wide angle photographs. More than a pano viewer, more than a view
camera, with features of both. For Linux/Unix, Win32, and Mac systems
with OpenGL 2.0

Comment 1 Nicolas Chauvet (kwizart) 2009-07-01 14:14:43 UTC
- starting review -

Comment 2 Nicolas Chauvet (kwizart) 2009-07-01 14:36:40 UTC
A new version is here : 0.71.100

%prep
try to keep timestamp when using %{__sed} -i 's/\r//'

It shouldn't be needed to use CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS to use our opt flags(at least on recent Fedora).

License Field is GPLv2+ (which match source code header) but GPLv3 text file is redistributed. (need to check lastest version)

Please submit .desktop and icon upstream or pick the one from the package if available.

Try to have the compiled binary / pixmaps installed with make install at %install step.

Comment 3 Ankur Sinha (FranciscoD) 2009-07-07 08:14:03 UTC
(In reply to comment #2)
> A new version is here : 0.71.100
> 
> %prep
> try to keep timestamp when using %{__sed} -i 's/\r//'

Done

> 
> It shouldn't be needed to use CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS to
> use our opt flags(at least on recent Fedora).
> 

removed

> License Field is GPLv2+ (which match source code header) but GPLv3 text file is
> redistributed. (need to check lastest version)
> 

GPLv3. Its been clarified in the latest version.

> Please submit .desktop and icon upstream or pick the one from the package if
> available.

Submitted them. If the upstream does not provide them, I need to make them ? I got them from http://hanzlici.cz/packages/fedora/panini/ this time.

> 
> Try to have the compiled binary / pixmaps installed with make install at
> %install step.  

I'm not sure on how to do this. The package generates a makefile using qmake-qt4. I need to patch this generated makefile before make uses it??

the srpm: 

http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm

the spec:

http://ankursinha.fedorapeople.org/panini/Panini.spec

all other files, (logs and the desktop and icon etc.) are at 
http://ankursinha.fedorapeople.org/panini/

regards,

Ankur

Comment 4 Nicolas Chauvet (kwizart) 2009-07-07 09:28:46 UTC
(In reply to comment #3)
> (In reply to comment #2)
>> License Field is GPLv2+ (which match source code header) but GPLv3 text file is
>> redistributed. (need to check lastest version)
>> 

>GPLv3. Its been clarified in the latest version.
How ? from the source code header, none are tag as GPLv3 but GPLv2+, with the exeption of pvQt_QTVR.{cpp,h} which are licensed as LGPLv2+
The binary package will ends to be GPLv2+ then! But since the GPLv3 is available from the svn repository, this will be more accurate to use GPLv3+ in the license fied anyway. So OKay.

...
> > Try to have the compiled binary / pixmaps installed with make install at
> > %install step.  
> 
> I'm not sure on how to do this. The package generates a makefile using
> qmake-qt4. I need to patch this generated makefile before make uses it??
You will need to patch the panini.pro file.
> the srpm: 
> 
> http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm
I don't understand why having %global	fname	panini, then fname is only used once?! But this remains trivial.
Also, you could consider to clean the old comments while importing.

FE-Legal:
From https://bugzilla.redhat.com/show_bug.cgi?id=503252#c3 my understanding is that we aren't really creating panorama with this tool, but we are extracting perspective from already made panorama pictures. Hence, this doesn't fall under 
any patent anyone may beleive to have.
But I would like a confirmation from FE-Legal about this...

Comment 5 Ankur Sinha (FranciscoD) 2009-07-07 09:49:18 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> >> License Field is GPLv2+ (which match source code header) but GPLv3 text file is
> >> redistributed. (need to check lastest version)
> >> 
> 
> >GPLv3. Its been clarified in the latest version.
> How ? from the source code header, none are tag as GPLv3 but GPLv2+, with the
> exeption of pvQt_QTVR.{cpp,h} which are licensed as LGPLv2+
> The binary package will ends to be GPLv2+ then! But since the GPLv3 is
> available from the svn repository, this will be more accurate to use GPLv3+ in
> the license fied anyway. So OKay.
> 

i was referring to this in the release file (panini-0.71-release.txt):

"LICENSE

Panini 0.7 is free software, copyright (C) 2008 - 2009 Thomas K Sharpless.
You can redistribute and/or modify it under the terms of the GNU General Public
License as published by the Free Software Foundation; either version 3 of the 
License, or (at your option) any later version."

Hadn't checked the source files. I'll try contacting upstream to clarify.


> ...
> > > Try to have the compiled binary / pixmaps installed with make install at
> > > %install step.  
> > 
> > I'm not sure on how to do this. The package generates a makefile using
> > qmake-qt4. I need to patch this generated makefile before make uses it??
> You will need to patch the panini.pro file.

okay. I'll try that. might take me 2-3 days to learn up qt.

> > the srpm: 
> > 
> > http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm
> I don't understand why having %global fname panini, then fname is only used
> once?! But this remains trivial.

I had used it more earlier. will remove it.

> Also, you could consider to clean the old comments while importing.

will do that.

> 
> FE-Legal:
> From https://bugzilla.redhat.com/show_bug.cgi?id=503252#c3 my understanding is
> that we aren't really creating panorama with this tool, but we are extracting
> perspective from already made panorama pictures. Hence, this doesn't fall under 
> any patent anyone may beleive to have.
> But I would like a confirmation from FE-Legal about this...  

okay

I'll do the other work until then

regards,

Ankur

Comment 6 Bruno Postle 2009-07-07 16:26:22 UTC
[not a review, just trying to help]

I have a spec for an earlier version of Panini here: http://bpostle.fedorapeople.org/reviews/Panini/Panini.spec

You might find some useful stuff, in particular you are not running update-desktop-database in %post and %postun (ignore the update-mime-database bit, I have it wrong).

I've spoken to upstream and there is no 'install' target, he doesn't have a use for one.

You are mixing macro styles, this is frowned upon by the guidelines: $RPM_BUILD_ROOT%{_bindir} should be something like %{buildroot}/%{_bindir}

My version of the %description has more keywords, in particular for searching you really need 'hugin', 'vedutismo', 'stereographic', 'QuickTimeVR' and 'QTVR'. You should also try and get the singular 'panorama' and the alternative 'Pannini' spelling into the description:

> Panini can load most common photo and panoramic formats from image files such
> as those created with hugin or QuickTimeVR (QTVR .mov) files.  Like all panorama
> viewers, it then shows a linear perspective view that can be panned and zoomed.
> But Panini can also display a range of wide angle perspectives via the
> stereographic and "Pannini" vedutismo families of projections, and shift,
> rotate, and stretch the image like a software view camera.

Comment 7 Ankur Sinha (FranciscoD) 2009-07-08 15:37:24 UTC
(In reply to comment #6)
> [not a review, just trying to help]
> 
> I have a spec for an earlier version of Panini here:
> http://bpostle.fedorapeople.org/reviews/Panini/Panini.spec
> 
> You might find some useful stuff, in particular you are not running
> update-desktop-database in %post and %postun (ignore the update-mime-database
> bit, I have it wrong).
> 

Thanks for this.I've added this portion.However, I don't know what the command is responsible for yet. Do I need to add the update-mime-database thing too?

I don't completely understand it from here. for eg, what is a "MimeType key."?

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database

> I've spoken to upstream and there is no 'install' target, he doesn't have a use
> for one.

Do I still need to patch the .pro?
> 
> You are mixing macro styles, this is frowned upon by the guidelines:
> $RPM_BUILD_ROOT%{_bindir} should be something like %{buildroot}/%{_bindir}
> 

Corrected :)

> My version of the %description has more keywords, in particular for searching
> you really need 'hugin', 'vedutismo', 'stereographic', 'QuickTimeVR' and
> 'QTVR'. You should also try and get the singular 'panorama' and the alternative
> 'Pannini' spelling into the description:
> 
> > Panini can load most common photo and panoramic formats from image files such
> > as those created with hugin or QuickTimeVR (QTVR .mov) files.  Like all panorama
> > viewers, it then shows a linear perspective view that can be panned and zoomed.
> > But Panini can also display a range of wide angle perspectives via the
> > stereographic and "Pannini" vedutismo families of projections, and shift,
> > rotate, and stretch the image like a software view camera.  

I changed the spec to this one.. 

new spec and srpm:

http://ankursinha.fedorapeople.org/panini/Panini.spec

http://ankursinha.fedorapeople.org/panini/Panini-0.71.102-1.fc12.src.rpm

all the other mock build results are here (rpms etc.):

http://ankursinha.fedorapeople.org/panini/

Comment 8 Tom "spot" Callaway 2009-07-14 21:13:16 UTC
Okay, so there are two issues here:

1. Since the code in the tarball all says GPLv2+ or LGPLv2+, and the readme says GPLv3+ (one of them does, another one says GPLv2+), and the latest SVN code (not just the readme) says GPLv2+ or LGPLv2+, we should email upstream and ask them to correct the license attributions in the code in SVN, and once that is done, we should be fine to use License: GPLv3+. Otherwise, if you don't want to wait, use License: GPLv2+ now and update it when and if they release something with proper code attribution.

2. Patent issues. The only patents I'm aware of are around control point generators for stitching. This code does not seem to do that, it seems to use already created panoramics. Can you confirm that?

Comment 9 Bruno Postle 2009-07-14 22:17:55 UTC
> However, I don't know what the command is responsible for yet. Do I need to
> add the update-mime-database thing too?

You need to update the desktop database when you install a .desktop file.  You are not introducing a new mime-type so apparently it isn't necessary to update the mime database.

> I don't completely understand it from here. for eg, what is a "MimeType key."?

This is for file associations, users should be able to select these kinds of files in (e.g) nautilus and open them in Panini:

MimeType=video/quicktime;image/jpeg;image/tiff;image/png;

Comment 10 Bruno Postle 2009-07-14 22:34:29 UTC
(In reply to comment #8)
> 
> 2. Patent issues. The only patents I'm aware of are around control point
> generators for stitching. This code does not seem to do that, it seems to use
> already created panoramics. Can you confirm that?  

Yes, Panini uses an already complete single panorama as input.

Comment 11 Nicolas Chauvet (kwizart) 2009-07-16 08:06:35 UTC
Someone has sent an email upstream already ?

We haven't leaved Fe-Legal, does patents around panoramic image are still searched, or it just remains the License question ?

Comment 12 Tom "spot" Callaway 2009-07-16 14:32:53 UTC
The only remaining issue is the License question. I have not reached out to upstream, I'd prefer if the package maintainer did that.

Comment 13 Ankur Sinha (FranciscoD) 2009-07-17 10:05:59 UTC
(In reply to comment #11)
> Someone has sent an email upstream already ?
> 

hi,

I have. Waiting for a reply. 

regards,

Ankur

Comment 14 Ankur Sinha (FranciscoD) 2009-07-17 15:10:50 UTC
hi,

Quick response from upstream :)

new srpm : 

http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-1.fc10.src.rpm

spec :

http://ankursinha.fedorapeople.org/panini/Panini.spec

other results from the mock build:

http://ankursinha.fedorapeople.org/panini/

regards,

Ankur

Comment 15 Tom "spot" Callaway 2009-07-17 21:34:58 UTC
Upstream source is now consistent, but you're still using the wrong license tag.

Since all of the code says: 
 * This file is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 3 of the License, or
 * (at your option) any later version.

You need to use:

License: GPLv3+

Comment 16 Tom "spot" Callaway 2009-07-23 15:04:33 UTC
Hello? This should be an easy fix, then I can lift FE-Legal.

Comment 17 Ankur Sinha (FranciscoD) 2009-07-23 16:39:59 UTC
(In reply to comment #16)
> Hello? This should be an easy fix, then I can lift FE-Legal.  

hi spot,

Sorry to have kept you waiting. Just got my internet connection today(in college) . Will have it up for you max by tomorrow. Upstream's requested my to use the icon he's packed. I'll correct both the issues and upload. 

regards,

Ankur

Comment 18 Ankur Sinha (FranciscoD) 2009-07-24 04:11:34 UTC
hi,

spec:

http://ankursinha.fedorapeople.org/panini/Panini.spec

SRPM:

http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-2.fc10.src.rpm

I've tried to use upstream's jpg file as the icon: I changed the .desktop file[1] and tested it. It *din't* work. How is one supposed to do it? I've already read https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage . It doesn't have much info on the .desktop.

[1]http://ankursinha.fedorapeople.org/panini/Panini.desktop

Other results from the mock build (logs etc.) : http://ankursinha.fedorapeople.org/panini/

regards,

Ankur

Comment 19 Julian Aloofi 2009-07-24 10:43:41 UTC
There is a company called Panini, if that matters:
http://www.paninigroup.com/

Comment 20 Tom "spot" Callaway 2009-07-24 11:44:49 UTC
"The image files must be one of the types: PNG, XPM, or SVG, and the extension must be ".png", ".xpm", or ".svg" (lower case)."

http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html

Since your image file is JPG, that's why it isn't working. Convert it to a PNG and it should be fine.

Lifting FE-Legal.

Comment 21 Ankur Sinha (FranciscoD) 2009-07-25 05:03:50 UTC
(In reply to comment #18)
> hi,
> 
> spec:
> 
> http://ankursinha.fedorapeople.org/panini/Panini.spec
> 
> SRPM:
> 
> http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-2.fc10.src.rpm
> 
> I've tried to use upstream's jpg file as the icon: I changed the .desktop
> file[1] and tested it. It *din't* work. How is one supposed to do it? I've
> already read
> https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage
> . It doesn't have much info on the .desktop.
> 
> [1]http://ankursinha.fedorapeople.org/panini/Panini.desktop
> 
> Other results from the mock build (logs etc.) :
> http://ankursinha.fedorapeople.org/panini/
> 
> regards,
> 
> Ankur  

hi,

Spec :

http://ankursinha.fedorapeople.org/panini/Panini.spec

SRPM:

http://ankursinha.fedorapeople.org/panini/Panini-0.71.103-3.fc10.src.rpm

other files at : http://ankursinha.fedorapeople.org/panini/

- icon works.
- seems to have packaged up correctly.
- no errors/warnings from rpmlint

regards,

Ankur

Comment 22 Nicolas Chauvet (kwizart) 2009-07-29 11:12:03 UTC
OK!


Panini is APPROVED 
----------------------------------

Comment 23 Ankur Sinha (FranciscoD) 2009-07-29 11:43:01 UTC
New Package CVS Request
=======================
Package Name: Panini
Short Description: A  tool for creating perspective views from panoramic and wide angle images
Owners: ankursinha
Branches: F-10 F-11 
InitialCC:

Comment 24 Jason Tibbitts 2009-07-29 15:14:37 UTC
CVS done.

Comment 25 Fedora Update System 2009-08-01 06:47:30 UTC
Panini-0.71.103-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/Panini-0.71.103-3.fc11

Comment 26 Fedora Update System 2009-08-01 06:56:24 UTC
Panini-0.71.103-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/Panini-0.71.103-3.fc10

Comment 27 Fedora Update System 2009-08-01 23:58:57 UTC
Panini-0.71.103-3.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update Panini'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-8203

Comment 28 Fedora Update System 2009-08-02 00:00:50 UTC
Panini-0.71.103-3.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update Panini'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-8211

Comment 29 Fedora Update System 2009-08-17 21:52:44 UTC
Panini-0.71.103-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2009-08-17 22:04:06 UTC
Panini-0.71.103-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.