Bug 58906

Created attachment 72361 [details] [review]
initial attempt at openjpeg2 support

This is an initial attempt at openjpeg 2 support. I've created JPEG2000Stream2 .cc files for the new openjpeg2 API but it should probably be merged into JPEG2000Stream.cc with some #ifdefs.

The build conf changes are incomplete due to the lack of pkg-config support in 2.0. I only created the minimal changes to get it to compile. LIBOPENJPEG_CFLAGS needs to be specified when running configure.

I found some additional regressions in openjpeg 2.0:
http://code.google.com/p/openjpeg/issues/detail?id=206

in addition to the regressions already in 1.5.1:
http://code.google.com/p/openjpeg/issues/detail?id=205

and the missing pkgconfig support:
http://code.google.com/p/openjpeg/issues/detail?id=68

So for now I recommend using openjpeg 1.5.0. When there is a new version that fixes the regressions and restores pkgconfig support I'll finish off this patch.

Created attachment 85114 [details] [review]
new attempt for openjpeg2 support

When try to render  when rendering http://www.eci.org/_media/downloads/altona_test_suite/eci_altona-test-suite-v2_technical2_x4.pdf with Your patch for openjpeg2, I got a huge number of error messages and no one of the jp2 compressed images were shown. I tried it also with the actual master version of openjpeg 2.x available over svn, but then I got immediately an assert.

1. I looked a little bit around and found out, that we have to set the user_data_length with opj_stream_set_user_data_length to avoid the assert, therefore I went back to read the complete jp2 stream into memory first to get a correct length for the stream.
2. Then I discovered that we have to implement the skip_function and have to return always the input value, even if it tries to skip over the end of the stream.

With these corrections I was able to render the PDF nearly completely, only two images are missing in the rendered output. But these two images are images in LAB colorspace, and it seems, as if openjpeg has no support for LAB images. 

@Adrian: I don't know which PDFs causes Your regressions, but I think that there is a great chance that my patch solves that, too. So it would be nice if You can tell me what are the problematic PDFs and / or test my patch with these PDF again.

Do you guys think its worth trying to share code between the two openjpeg implementations? I did a quick diff and i'm undecided about if it's a good idea or not, so since you guys have probably looked more at it, what do you think?

(In reply to comment #3)
> Do you guys think its worth trying to share code between the two openjpeg
> implementations? 

That is my intention. I'm still waiting for openjpeg to release a new version that restores pkg-config support before I finished this patch. It has been fixed in their subversion repository.

Mabe ping them in their mailing list/personal mail? With something like a "Yo guys, we really need this, it'd be cool if you did a release"

Hi, since the last comment, the openjpeg2 pkg-config support issue has been closed and there was mention of backporting the fix, but no action has been taken.
http://code.google.com/p/openjpeg/issues/detail?id=68

Created attachment 98733 [details] [review]
rebased  new attempt for openjpeg2 support

I rebased my new attempt for openjpeg2 support, because I need it to continue working on bug 68986.

@Adrian: in April they released openjpeg 2.1. Are You able to have a look if the pkg-config support is now available again? Should be according to the issue tracker. If so, can we try to work on a final fix for this bug?

(In reply to comment #7)
> @Adrian: in April they released openjpeg 2.1. Are You able to have a look if
> the pkg-config support is now available again? Should be according to the
> issue tracker. If so, can we try to work on a final fix for this bug?

I can have a look. Can you provide your changes as a separate git commit instead of combined with my commit.

(In reply to comment #8)
> (In reply to comment #7)
> > @Adrian: in April they released openjpeg 2.1. Are You able to have a look if
> > the pkg-config support is now available again? Should be according to the
> > issue tracker. If so, can we try to work on a final fix for this bug?
> 
> I can have a look. Can you provide your changes as a separate git commit
> instead of combined with my commit.

Should I merge it JPEG2000Stream.cc with some #ifdefs instead of using separate files?

Created attachment 98748 [details] [review]
initial attempt at openjpeg2 support

I've updated my patch to support openjpeg2 with pkg-config support. I've also merged the JPEG2000Stream2.cc changes into JPEG2000Stream.cc.

Thomas, you should be able to rebase your changes onto this.

(In reply to comment #10)
> Created attachment 98748 [details] [review] [review]
> initial attempt at openjpeg2 support
> 
> I've updated my patch to support openjpeg2 with pkg-config support. I've
> also merged the JPEG2000Stream2.cc changes into JPEG2000Stream.cc.
> 
> Thomas, you should be able to rebase your changes onto this.

I first just applied Your patch, the configure now works fine, it detects openjpeg 2.1. BUT: when I try to compile it (without any changes from me), I got

JPEG2000Stream.cc: In member function 'void JPXStream::init()':
JPEG2000Stream.cc:231:3: warning: 'void opj_stream_set_user_data(void**, void*)' is deprecated (declared at /usr/local/include/openjpeg-2.1/openjpeg.h:1082) [-Wdeprecated-declarations]
JPEG2000Stream.cc:231:47: error: too many arguments to function 'void opj_stream_set_user_data(void**, void*)'
In file included from JPEG2000Stream.h:21:0,
                 from JPEG2000Stream.cc:17:
/usr/local/include/openjpeg-2.1/openjpeg.h:1082:1: note: declared here
JPEG2000Stream.cc:231:47: warning: 'void opj_stream_set_user_data(void**, void*)' is deprecated (declared at /usr/local/include/openjpeg-2.1/openjpeg.h:1082) [-Wdeprecated-declarations]
make[3]: *** [JPEG2000Stream.lo] Fehler 1

I looked into the openjpeg 2,0 header files, it's the same, opj_stream_set_user_data only accepts two arguments, so I'm wondering how You get it compiled. Okay, if I apply my changes, we'll get it compiled again. But will we get my patch compiled on any installation???

Created attachment 98764 [details] [review]
initial attempt at openjpeg2 support

I merged now my changes to Adrian's patch. I think, we can give it a try and commit it to master if nobody finds a problem.

Created attachment 98805 [details] [review]
Initial attempt at openjpeg2 support

The opj_stream_set_user_data function has changed between 2.0.1 and 2.1.0. I've updated my patch to handle this difference. I've also moved openjpeg.h into the .cc file to avoid polluting the global namespace and added an OPENJPEG_VERSION macro to make it easier to check the version as I don't think this will be the last time that openjpeg breaks API.

(In reply to comment #12)
> Created attachment 98764 [details] [review] [review]
> initial attempt at openjpeg2 support
> 
> I merged now my changes to Adrian's patch. I think, we can give it a try and
> commit it to master if nobody finds a problem.

I suggest attaching your changes as a separate commit so your code gets committed under your name and my code gets committed under my name.

> +  unsigned char *buf = str->toUnsignedChars(&length, bufSize);

Where does this get freed? Looks like a memory leak.

(In reply to comment #14)
> (In reply to comment #12)
> > Created attachment 98764 [details] [review] [review] [review]
> > initial attempt at openjpeg2 support
> > 
> > I merged now my changes to Adrian's patch. I think, we can give it a try and
> > commit it to master if nobody finds a problem.
> 
> I suggest attaching your changes as a separate commit so your code gets
> committed under your name and my code gets committed under my name.

Ok, I'll wait until Your code is committed and then look if my changes are still necessary.

> 
> > +  unsigned char *buf = str->toUnsignedChars(&length, bufSize);
> 
> Where does this get freed? Looks like a memory leak.

Thanks for the hint. I'll keep that in mind if my code changes are still needed.

(In reply to comment #15)
> Ok, I'll wait until Your code is committed and then look if my changes are
> still necessary.

My patch won't work without your changes so we need your patch before mine can be committed to allow testing with openjpeg2.

(In reply to comment #16)
> 
> My patch won't work without your changes so we need your patch before mine
> can be committed to allow testing with openjpeg2.

So You mean I should do a git diff against Your changes so we see only my changes on top of Yours? I'm not really experienced in git, but I'll try to find out how to do it on monday (I'm busy with other things this weekend)

git clone git://git.freedesktop.org/git/poppler/poppler
git config user.name "your name"
git config user.email "your@email.com"
git checkout -b openjpeg # create new branch and switch to it
git am < adrian.patch # import my commit
<make your changes>
git commit -a # commit your changes
git format-patch HEAD^ # export last commit as a patch (use HEAD~n for last n commits)

Hello,

I was trying to completely remove openjpeg 1 from my computer these days, and I was so lucky to find this work. (What a coincidence!) Thanks for your patches! That helped a lot.

And here is a bug report for the patch. It does not work on the file http://fanat1k.ru/tmp/test_pdf.pdf (It was from this post https://github.com/mozilla/pdf.js/issues/2788.)

The reason is the file needs OPJ_CODEC_J2K, but jpxData->pos was not set to 0 when OPJ_CODEC_JP2 decoding failed and started to try OPJ_CODEC_J2K. Maybe you can move the jpxData definition (and stream) to init2, and that should solve the problem. (If I'm not wrong, probably you might also need to release variables stream and decoder after the "error:" line to avoid memory leak.)

A better solution is to determine the format before decoding. I checked some codes from ghostscript. If the first two bytes of buf is 0xFF, 0x4F, then use OPJ_CODEC_J2K; otherwise use OPJ_CODEC_JP2. We can safely ignore OPJ_CODEC_JPT, because it is not implemented in openjpeg. (See the code of opj_create_decompress in libopenjpeg/openjpeg.c in opengjpeg 1.5.1 or src/lib/openjp2/openjpeg.c in openjpeg 2.1.0.)

I attached the JPEG2000Stream.cc file I'm using. (It is basically your patch with some tweaks.) I also removed opj_end_decompress (is it useful?) and restored the skip function to the way it should be (it should return jpxData->pos instead of skip). And it works perfectly on the above test_pdf.pdf file. There are also no openjpeg errors for the pdf file in provided "Comment #2" (but there is an error from poppler, which seems doesn't matter). Hope this will be useful.

Created attachment 98827 [details]
See Comment #19

Created attachment 98833 [details] [review]
extended openjpeg2 support

Thanks to Adrian: this patch is now created on top of Adrian's patch and it solves the problems of comment 2.
I also free buf now.
Last but not least thanks to tarlou.gd@gmail.com: with this hint I encountered that at least with openjpeg2 opj_stream_t cannot be used twice with different encoder, therefore I create now always a new one, so this patch also renders the PDF of comment 19

Albert, these patches are ready for review.

I know, sorry i've been terribily busy lately, it's on my list of things to regtest

Any update on the review?

I can nothing but excuse myself again, i'll put it at the top of my todo.

The cmake build system seems to be broken (still have not tried to use openjpeg2, just openjpeg1), I get

Linking CXX executable pdfdetach
../libpoppler.so.46.0.0: undefined reference to `JPXStream::init()'

Please fix it.

I'll try to compile using auto* and test

Hmmm, you are using pkgconfig to check for openjpeg but my openjpeg donwload as no pkgconfig file, how does that work for you guys?

Openjpeg 2.0.0 has missing pkgconfig support. pkgconfig support was restored in subsequent versions (2.0.1 and 2.1.0).

As 2.0.0 has some serious regressions compared with the 1.x versions I decided it was not worth adding the extra complexity to the build files to detect that version.

Where do you get those? I can't find them at https://code.google.com/p/openjpeg/downloads/list :S

They have changed their download location but have not updated all their links. Click on "News" on the left side of the home page.

Downloads are at https://code.google.com/p/openjpeg/wiki/Downloads

Created attachment 106920 [details] [review]
cmake

I am not familiar with cmake. I managed to copy and paste together something that seems to work.

Various issues:

 * Rendering the pdf that i will attach with opengjpeg2 renders a much blurrier text at the bottom than with openjpeg1, can you have a look as to why that happens?

* Using configure and openjpeg2 in an non standard dir i get

/home/tsdgeos/devel/poppler/glib/tmp-introspect13BDMh/.libs/lt-Poppler-0.18: error while loading shared libraries: libopenjp2.so.7: cannot open shared object file: No such file or directory


* The cmake support is still not good, i hacked it up to work, don't really remember the changes, i'm attaching the whole files, and after everything else is solved i guess i can just copy&paste or something http://paste.kde.org/pn1ufckym http://paste.kde.org/ph1jbuvn0 http://paste.kde.org/pxbkd570d http://paste.kde.org/pt9tiqs4s

Created attachment 107013 [details]
PDF blurrier in openjpeg2 codepath than openjpeg1

(In reply to comment #33)
> Created attachment 107013 [details]
> PDF blurrier in openjpeg2 codepath than openjpeg1

I see what You mean. But in my eyes it is a change in the openjpeg implementation. Do the following:

./utils/pdfimages -all moo.pdf ./moo-img/moo

and the with openjpeg 1:

j2k_to_image -i ./moo-img/moo-000.jp2 -o moo-oj1.tif

and with openjpeg 2:

opj_decompress -i ./moo-img/moo-000.jp2 -o moo-oj2.tif

You'll see the same differences in the output as You see with poppler. And I don't find any way to get it decompressed with version 2 as it was done with version 1.

(In reply to comment #33)
> Created attachment 107013 [details]
> PDF blurrier in openjpeg2 codepath than openjpeg1

Seems to be https://code.google.com/p/openjpeg/issues/detail?id=254

(In reply to comment #35)
> (In reply to comment #33)
> > Created attachment 107013 [details]
> > PDF blurrier in openjpeg2 codepath than openjpeg1
> 
> Seems to be https://code.google.com/p/openjpeg/issues/detail?id=254

I can confirm that it the patch of comment #12 solves it

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #33)
> > > Created attachment 107013 [details]
> > > PDF blurrier in openjpeg2 codepath than openjpeg1
> > 
> > Seems to be https://code.google.com/p/openjpeg/issues/detail?id=254
> 
> I can confirm that it the patch of comment #12 solves it

Of course https://code.google.com/p/openjpeg/issues/detail?id=254#c12 is meant, not our comment #12

Asked if those patches are going to make it to a release, at this point, does it make sense to pick openjpeg2 over openjpeg1 when we know it gives us less quality?

(In reply to comment #38)
> Asked if those patches are going to make it to a release, at this point,
> does it make sense to pick openjpeg2 over openjpeg1 when we know it gives us
> less quality?

1. According the comments on the openjpeg bug the jp2 image included the PDF You attached is invalid, s. https://code.google.com/p/openjpeg/issues/detail?id=254#c11
2. If we wait until all "bugs" are solved we can wait at the Greeks calends :-) , and probably support of openjpeg 1 will be suspended earlier.
3. Some other bugs /features are solved i.e. in openjpeg 2.1 which will no more be fixed in openjpeg 1, see i.e. eci altona testsuite v2.
4. But I think we can discuss about what openjpeg version should be used by default if both are installed

(In reply to comment #32)
> * Using configure and openjpeg2 in an non standard dir i get
> 
> /home/tsdgeos/devel/poppler/glib/tmp-introspect13BDMh/.libs/lt-Poppler-0.18:
> error while loading shared libraries: libopenjp2.so.7: cannot open shared
> object file: No such file or directory

I can reproduce this on Debian Testing if I install openjpeg2 in a directory that is not /usr/local or the directory I install poppler into.

This problem is beyond my understanding of automake and libtool. From what I can find out on google this is not caused by anything in my patch but is caused by the Debian libtool which has been modified to disallow -rpath to non standard locations (https://wiki.debian.org/RpathIssue).

It seems to be this setting that is causing the problem:
 $ grep link_all_deplibs= libtool
 link_all_deplibs=no

Compiling and installing an unpatched libtool fixes the problem.

Other solutions include putting the openjpeg2 lib in LD_LIBRARY_PATH or installing it in the same prefix as poppler.

There seems to be some movement at https://code.google.com/p/openjpeg/issues/detail?id=254 that's good

I know openjpeg2 can have some fixes/features versus 1, but sincerely i'd prefer we at least don't regress and prefer 1 over 2 (for the moment) so people stay at least as they are in the pdfs they had.

what everyone else thinks?

(In reply to Albert Astals Cid from comment #41)
> There seems to be some movement at
> https://code.google.com/p/openjpeg/issues/detail?id=254 that's good
> 
> I know openjpeg2 can have some fixes/features versus 1, but sincerely i'd
> prefer we at least don't regress and prefer 1 over 2 (for the moment) so
> people stay at least as they are in the pdfs they had.
> 
> what everyone else thinks?

That would be okay for me. 

But just another idea: openjpeg 2.x is not available as debian package in the moment. So if someone want to use it he has to download the sources, compile and install it. This should be done only on a development system, and a developer should know what he does :-) So in my eyes: if I have installed it I also want to use it.

On a "normal" system still only openjpeg 1.3 is installed.

Created attachment 107553 [details] [review]
Initial attempt at openjpeg2 support

> I know openjpeg2 can have some fixes/features versus 1, but sincerely
> i'd prefer we at least don't regress and prefer 1 over 2 (for the
> moment) so people stay at least as they are in the pdfs they had.

I agree that regressions are unacceptable. I've updated my patch to default to openjpeg1 and provide the option to select openjpeg2.

Albert, ping

I know, i know, sorrry. It's on the todo list for xmas where i will hopefully have some time.

I'm going to complain about broken cmake again, while i find out how the hell to use auto* i haven't used for a long time to test the rest of the patch.

That was a bit too rude, excuse my bad xmas manners.

Ok, so i've run the reg test with openjpeg1 and all is fine, openjpeg2 code asserts in lots of places.

I guess is how we want to proceed. I'd suggest:
 * Get the cmake code done
 * Commit
 * Create a master "openjpeg2 regresses" bug in our bugzilla
 * Create a child bug for every file that i have that regresses
 * Try to get the openjpeg2 devels to fix openjpeg.

Comments?

(In reply to Albert Astals Cid from comment #48)
> Ok, so i've run the reg test with openjpeg1 and all is fine, openjpeg2 code
> asserts in lots of places.
> 
> I guess is how we want to proceed. I'd suggest:
>  * Get the cmake code done
>  * Commit
>  * Create a master "openjpeg2 regresses" bug in our bugzilla
>  * Create a child bug for every file that i have that regresses
>  * Try to get the openjpeg2 devels to fix openjpeg.
> 
> Comments?

That's ok for me. Just one question: which openjpeg2 code do You use? As far as I remember when I used the subversion checkout there are not so many asserts in my regression test.

> That's ok for me. Just one question: which openjpeg2 code do You use? As far
> as I remember when I used the subversion checkout there are not so many
> asserts in my regression test.

I'm using 2.1.0-2 as packaged in ubuntu

Ok, i've pushed everything and made the tweaks for the cmake code myself.

Will close this one and start a reg test against openjpeg2 as said in one of the comments.

New master bug at https://bugs.freedesktop.org/show_bug.cgi?id=88030