search

jai-imageio / jai-imageio-core

JAI ImageIO Core (without javax.media.jai dependencies)
Other
234 stars 87 forks source link

Merging another jai fork #33

Closed rleigh-codelibre closed 7 years ago

rleigh-codelibre commented 7 years ago

Hi,

Our group has long maintained a jai fork here: https://github.com/openmicroscopy/bioformats/tree/develop/components/forks/jai

We are in the process of modularising our codebase and this would be an opportunity to drop our jai fork entirely, once we've verified your fork doesn't cause any regressions. Given the original upstream no longer maintains jai, this project seems like it's the next best thing as the new de-facto upstream project.

I'd just like to ask if it would be acceptable to rebase and open PRs for all of the outstanding fixes in our fork, which recently includes java9 support, and also if it would be possible to join the organisation so we can more closely participate in its ongoing maintenance, since we heavily rely upon it. It looked like it went through a quiet period with little activity for a few months, so we weren't sure how actively it was maintained, though it seems to have been fairly active recently.

Kind regards, Roger Leigh

stain commented 7 years ago

Yes, it would certainly make sense to join effort!

Which usernames should I add to the jai-imageio organization? I've invited @rleigh-codelibre @sbesson @jburel - I've generally been inviting anyone who raised a pull request.

Agree on the process of doing individual Pull Request per fix - as you have jai-imageio as part of a larger repository and have not split out JPEG2000 support - so, the best would be if you are able to reconstruct which fixes you have done and reapply them as PRs to jai-imageio-core and/or jai-imageio-jpeg2000.

Note that changes to the jpeg2000 code has to be more carefully reviewed, as if the change breaks JPEG2000 compatibility as it could be patent infringing

stain commented 7 years ago

Note that although I invited you all, I still think it's good to work by PR first for any change, as it gives good logs, email notifications and a chance to review. If you've not heard anything in a week, merge yourself.

rleigh-codelibre commented 7 years ago

@stain Thank you very much for inviting us all, it's very much appreciated. We'll need to go through a decade+ of git history and identify each change we've made from its initial import, and then reapply or recreate that change on top of the current jai-imageio-(core|jpeg2000), so we can certainly keep each fix as a separate pull request. It may well be the case that some of them are already fixed by yourselves!

We have a large corpus of test data, plus all our code based upon jai-imageio, so we will be able to test a large part of jai-imageio against this test data, which should pick up any regressions once we've merged all the outstanding fixes and are in a position to be able to swap our fork for yours.

Thanks again, Roger

(I'll close the issue, since it seems there's not anything outstanding on it. I hope we'll be able to open some PRs soon, but it might take a while to plough through all the git history and redo the changes!)

stain commented 7 years ago

Thanks! Welcome to the project!

Do you think some of these changes are immediate? I was thinking of cutting a release soon to get the #22 fix out.

rleigh-codelibre commented 7 years ago

I've done a preliminary git filter-branch and put the results here:

https://github.com/rleigh-codelibre/jai-split and https://github.com/rleigh-codelibre/jai-split-minimal -- the only difference is one is more aggressively filtered, I'll need to manually check the content of both to be sure my filtering hasn't missed anything.

I need to find the parent JAI commit to get a diff for the initial addition, which adds J2K YCbCr support. It's likely the previous one for that date (16th Oct 2008).

I don't know if any are immediately needed, because I don't yet know myself what the content of the changes is--still working on that, I'll know more once I've reviewed the content of the above branches. So should be OK to leave for the next release.

rleigh-codelibre commented 7 years ago

@stain I've done some further work to rebase all the functional changes onto the original SVN commit the fork originated from, and split out the jj2000 changes into separate commits. This is at https://github.com/rleigh-codelibre/jai-tidy/commits/master and includes all the attributions for the original authors. It's pending some further review, and not all of the commits are suitable or useful to pick up. But if you wanted to pick up the more trivial changes prior to us opening PRs for bits that might require more work to integrate and test, it's there if you wanted to take a look over it. Quite possible a number of the minor fixes are already fixed by yourselves.

I'm away next week, but should be able to start properly feeding the useful changes back to you once I've re-rebased them onto your master the following week. Edit: I've opened a few PRs where the intent was obvious and they applied cleanly.

stain commented 7 years ago

Great, @rleigh-codelibre! That looks like a tidy set of patches that is easy to work through.

Note before rebasing that there is the renaming of folders (to Maven standard src/main/java) and packages (com.github.jaiimageio) in this jai-imageio-core, so you may want to rebase to one of my early commit before files renamed (say 0b9d6d5f65bdd247dec0b42ebddb51d363c7206c)