what's needed to add jhead/jpegtran rotation support?
|
mattdm
Joined: 2005-07-22
Posts: 181 |
Posted: Mon, 2006-09-25 21:41
|
|
Okay, so, I just installed G2, hoping to migrate from G1, and the lack of lossless rotation is kind of a showstopper. I'm not a PHP coder (although I'm fluent in perl and know enough to hack at PHP), but since no one else seems to be doing it, I figured I might as well take a stab. Without having looked to deeply, I'm under the impression the way to this would be to make a jpegtran module which would implement GalleryToolkit and be able to perform rotate operations in performOperation(). But, uh, how does one tell gallery that if enabled, this toolkit should be used for rotation in preference to any other ones? And, if it's that easy, how come no one has done it already? I must be missing something. The other option is simply to hack the ImageMagick module so *it* looks for jpegtran and uses it if available instead of calling "convert -rotate". Clearly that's cheating, though. |
|
Posts: 32509
yep, pretty much.
you'd need to create a module that registers a toolkit for mimetype = jpeg and operation = rotation.
and then you'd need to make that toolkit the top priority in site admin -> toolkit priority.
it's just not a high priority, that's why noone has done that yet.
you can file a feature request and then vote for it such that it gets into the top requests at
http://gallery.menalto.com/sfvote/all .
Posts: 181
Thanks. I'll try to work on it this week.
For the priority, is there a way it can insert a message on the rotation edit page warning if it's *not* top priority?
PS: It's already filed, as number 1368463, with 93 votes.
Posts: 32509
excellent!
see:
http://codex.gallery2.org/index.php/Gallery2:Module_Development_Tutorial#Coding_a_Dummy_Module
i would start with a simple module that installs / activates. done in a few minutes.
then, write the toolkit class and register it in your performFactoryRegistrations() function in module.inc.
at this stage, your toolkit can have a hardcoded path to jpegtran.
if you'd like to share your module or even contribute it such that it goes into g2's official release, you'll want to make the path to the jpegtran binary configurable.
see dcraw, gd, imagemagick, netpbm as examples. the latter 3 certainly being among the more complex modules.
the latter two, together with ffmpeg, archiveupload, zipcart, help with the admin pages and activation code (configuration page asking for path to binary, saving the binary path, ..).
Posts: 181
Cool. I'll probably borrow liberally from the other modules -- particularly, the "find the dcraw binary" dialog is directly translatable to this.
Posts: 181
I note that get*ToolkitPriority says "managed priority range (20-40)", and that lower numbers = higher priority. Is it bad form to set priority to, say, "0"? The graphics toolkits I've looked at call getMaximumManagedToolkitPriority and then set priority to the next highest value (i.e. lowest priority). For my 0.0.1 release, I'm going to follow that convention, and you'll have to bump up the priority if you want it to actually, y'know, *work*.
Posts: 32509
you can set it to 20 on toolkit registration time. 20 is maximum priority.
Posts: 181
That's a bit surprising. Thanks.
What happens if multiple things are at 20?
Posts: 32509
that doesn't happen if you use getMaximumManagedToolkitPriority () or the toolkit priority admin view to manage them...
but if it happens, there's nothing in g2 to handle it specifically. one of the two will win. in mysql, usually the one that has been first inserted.
Posts: 181
So, what happens if I set it to 19? Everything breaks?
Posts: 181
Okay, got a first cut. (Set priority to 20, btw.) I'll post in a new thread.
Posts: 32509
Awesome!
your server is still down, so i couldn't actually look at the code.
some notes:
- Please do not alter the original file. that's not consistent with our framework. the caller can adjust it by srcfile = dstfile, but your toolkit shouldn't change the source file.
- exif: ensure to copy all embedded data (exif, xmb, iptc, ..): -copy all, see: http://www.gsp.com/cgi-bin/man.cgi?section=1&topic=jpegtran
- you can set the priority to 1. by default, we set it to 5. the 20-40 priority range is just the "managed" range, meaning that the API tries to put them into that range when you manually edit the priority order.
- other ops?
Posts: 181
Should be back now. Some hardware glitch.
I'm writing to $destFilename as passed to performOperation -- I think that does the right thing?
Yeah, I thought of that. Is there anything more than "-copy all" that should be done?
Setting it to 20 appears to make it beat ImageMagick by default. What're your thoughts on this?
It'd be pretty easy to add flipping and transposing in a later version.
Thanks for your help!
Posts: 32509
@priority:
just talked to mindless about that. if you use a priority outside the 20-40 range, the toolkit won't appear in the "site admin -> toolkit priority" page. users won't be able to adjust the priority.
i think that would be bad. users should know what's going on, which toolkit has priority over which.
right now, please hardcode "20" as priority, exactly what you do right now. once your module makes it into svn, we can clean that up, e.g. by adding a getMinimumManagedToolkitPriority() method (a minor core api version bump).
@exif and other data:
i guess "-copy all" is all that is needed.
@destfile:
looks good!
@using verbose:
do you know the performance difference between -verbose and omitting that option?
@other ops:
that would be cool!
Posts: 181
@priority:
Yeah, I discovered that, so I left it at 20. Thanks.
@exif data, etc:
thanks
@destfile:
Good, 'cause I'm just guessing as I go along.
@using verbose:
Seems negligible, although I haven't tested in depth. Probably okay to remove since it doesn't actually provide any useful information.
@other ops:
Can I just make up operations arbitrarily? I don't actually see a "flip" operation anywhere in the ImageMagick or NetPBM modules. (ImageMagick, by the way, use "flip" for horizontal, and "flop" for vertical.) I don't see transpose and transverse being useful at all, but hey, might as well add them.
The other thing jpegtran can do is convert to greyscale while preserving luminance exactly (no reencoding). This might be useful, although I can see wanting that to actually be separate from the transformation operations so you can set priority differently. (Maybe a whole different module?)
Posts: 32509
@verbose:
if there's no remarkable performance penalty, i'd keep it.
in case things don't work as expected, it might provide useful debug information.
@other ops:
the following operations are used in g2 (from the thoperatnmap table):
other operations are currently not used in the g2.
there's no flip operation in g2. rotating by 180 is flipping in your terms i guess.
useful would be: rotate and crop.
if you add operations that are not yet used, that's cool as well. future modules / features can then depend on such operations.
Posts: 181
so, this implements rotating 90, -90, 270, and 180 degrees. Jpegtran doesn't do anything like cropping, although it seems like it maybe *should*, in 8-pixel-wide blocks. Oh well.
Flipping is not the same as rotating 180 degrees -- it makes a mirror image. "Flopping" (in ImageMagick terminology) is an up-down mirror image. But these things aren't as generally useful in Gallery, 'cause it's not like your digital camera is, ooops, going to take a picture in reverse. (For what it's worth, G1 does implement flipping/flopping.) I think the transpose is simply rotating and flipping in one step.
So anyway, what this basically means is that the currently-useful features are already all implemented.
Do I need to do anything more to convince you to check it into the main tree?
Posts: 3236
Just for clarification, mirroring / flipping / flopping means that if you are looking at an image with a tree on the left and a person on the right, you "flip" it or "mirror" it and the tree is on the *right* and the person is on the left. *IF* one were scanning negatives or slides, I believe this could be used to make the image "correct", but it could also be for asthetics. Maybe someone feels the picture just *looks* better the other way around.
I think the "effects" might be good in another module. I think netpm, gd, im and jpegtran all add unique, interesting effects you could add. It might be interesting as an "artistic" module so that an image can be given a sepia or black/white (greyscale) effect or something. Probably *not* actually that popular, since anyone who *wants* to do that will do it with PS or what ever.
_________________________________
Support & Documentation || Donate to Gallery || My Website
Posts: 32509
i guess you're referring to the DCT block-size.
http://www.gsp.com/cgi-bin/man.cgi?section=1&topic=jpegtran says jpegtran also has crop and overlay (drop) features. and they're not limited to 8px blocks.
if i look at my jpegtran version (6b), it says:
so crop and overlay should be possible, i guess. but don't worry, it's not that important right now.
what else is needed to get your module into gallery?
- code review for code style
- general code review
- adjust the module to be coded against current svn instead of g2.1 (minor API changes):
http://codex.gallery2.org/index.php/Gallery2:API_Changes_Since_The_Last_Release
- adding tests (see modules/*/test/phpunit/*)
- testing on windows, linux
- DONE - if multiple jpegtran versions are common, test against those. check for argument incompatibilities
-> the last jpegtran version is from 1998, no need to check for it. i doubt our g1 code does version specific stuff for jpegtran.
- adding GalleryCoreApi::getMinimumManagedToolkitPriority(), bumping the core api to 7,11. changing jpegtran module.inc to require this core api version. using getMinimumManagedToolkitPriority() in performFactoryRegistration()
most of that isn't your job and must be done by a g2 core developer.
you can make our work easier / speed up the process by changing the code to be up to date with the latest API changes. and by adding tests mostly by copying from other toolkit modules.
i'll do a code readability review tonight.
Posts: 181
The jpegtran 6b I have (from RHEL4/CentOS/Fedora) does not have a crop option. So, I looked at http://www.ijg.org/ , and found that the official source doesn't either. This must be something added in the FreeBSD version....
Posts: 181
Yeah, looks like they added the patch from: http://sylvana.net/jpegcrop/jpegtran/
I can put in tests for that, and for other features like "drop" (the picture overlay). But I think it's best to just get this in as-is and worry about extending it later.
Posts: 32509
Nice find!
So we have multiple versions.
Yes, I concur. We can add features later. Rotation is the most important task and that should go in.
Posts: 181
Tracking down $%%# like "we patched in this new featureset without changing the version number or indicating in any way that we're nonstandard" is my day job.
Posts: 32509
Here's a review:
Overall, this is excellent!
Since we do pedantic code reviews, it's suprising to have to note so few things in a first review.
Just some minor comments:
- JpegtranToolkitHelper::testBinary(): please remove the $version return value. it's not used anywhere.
- therefore, you also don't need to set the module parameter "version"
- change the module version to 0.9.0, that's a reasonable number to start with
- add "image/jpeg" to the mimeType list for your operation, it might be useful at some point
- some edits as a consequence of that since you can no longer hardcode 'image/jpeg' in that case
- watch out for the max 100 chars / line code style limit (http://codex.gallery2.org/index.php/Gallery2:Coding_Standards)
- see lines starting with "## " in all files for my code comments. some minor things need to be changed.
attached is the reviewed module with inlined comments.
Posts: 181
Thanks. I'll go through this tomorrow.
As you can see, I borrowed a lot of code, which was why I was able to get going so quickly. That's also why I started with such a low version number -- I didn't know it'd come together so easily. In general, I'm impressed with the design decisions that make it easy to stitch together a module like this with relatively little work.
It also looks like dcraw could use some going over, huh?
Posts: 181
I'm not going to have a chance to do the code cleanup until next week sometime. *sigh*
Posts: 32509
np, but I'm still looking forward to seeing this module in g2!
Posts: 181
So, we may be having our baby a month earlier than expected. If I suddenly go completely AWOL, it might be good if we can find someone else to help with this.
Posts: 32509
I'll handle the baby, you finish the module. Ok?
Posts: 181
Oh, that's tempting!
But I'll *try* to do both....
Posts: 2254
Any update on this? I'm itching to close http://sourceforge.net/tracker/index.php?func=detail&aid=1410780&group_id=7130&atid=357130
--
http://ckdake.com/
If you found my help useful, please consider donating to Gallery.
Posts: 181
Well, as threatened, we had the baby. I figure it'll take about 3 hours of uninterrupted time to get the module into shape, not counting the time required to set up a svn-based gallery to develop against. I hope I'll be back to having three hours of interrupted time in a row by mid November.
Posts: 32509
Too bad
Then I'll have to do it.
I'll finish your module and get it into svn.
Update: Implemented crop, fixed stuff and it's soon in gallery-contrib. Then we'll add tests and it will go into main svn after 2.2.
Posts: 32509
Ok, it's in gallery-contrib now.
Users of G2.2 will be able to install it via site admin -> plugins -> more plugins (downloadable plugins).
Posts: 181
Thank you so much for finishing the job for me. And glad I could help get it started.