Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encode: do not perform rate control for single-tile lossless #1009

Closed
boxerab opened this issue Aug 31, 2017 · 17 comments
Closed

Encode: do not perform rate control for single-tile lossless #1009

boxerab opened this issue Aug 31, 2017 · 17 comments

Comments

@boxerab
Copy link
Contributor

boxerab commented Aug 31, 2017

Currently, for single tile lossless, rate control is performed, although it is not needed.

This can lead to lossy encoding if there are code passes at the end of a code block that do not
contribute to a rate increase. The rate control algorithm will remove these code passes,
leading to potential lossy encode.

@boxerab
Copy link
Contributor Author

boxerab commented Aug 31, 2017

Correct solution is to simply include all passes in a single layer for single-layer lossless

@rouault
Copy link
Collaborator

rouault commented Aug 31, 2017

Do you have examples where that occurs ?

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

No, I can't remember the exact case, but I saw this happening when I removed the limit of 10 resolution levels. Also, if you look at the logic in the code, you can see how this might happen.

But, even without a test image, there is no reason to perform rate control on single layer lossless image.
You just need a method to detect when this is the case, and skip the rate control.

@rouault
Copy link
Collaborator

rouault commented Sep 1, 2017

Also, if you look at the logic in the code, you can see how this might happen.

Actually I don't see how that could happen. Lossless encoding implies cp->m_specific_param.m_enc.m_disto_alloc == 1 and tcd_tcp->rates[0] == 0. Which implies in opj_tcd_rateallocate() that goodthresh = min; which is the minimum of dd / dr for all passes of all code blocks (

goodthresh = min;
). Then opj_tcd_makelayer() is called with this threshold and checks in
if (thresh - (dd / dr) <
DBL_EPSILON) { /* do not rely on float equality, check with DBL_EPSILON margin */
if (thresh - (dd / dr) < DBL_EPSILON), which should be always true. So all passes should be selected. Am I missing something ?

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

hmmmm. Well, I guess that was a false alarm then :)

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

How about lossless with multiple layers ? Could the final layer be missing some passes ? If so, that would be a problem.

rouault added a commit that referenced this issue Sep 1, 2017
@rouault
Copy link
Collaborator

rouault commented Sep 1, 2017

The same logic will apply for multiple layers. For lossless encoding, the last layer should have tcd_tcp->rates[layno] == 0, in which case all remaining coding passes will be selected. I just committed a fix for opj_compress help which indicated 1 instead of 0 for this lossless terminating layer

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

For -r flag, 1 indicates a final lossless layer, while for -q flag, 0 indicates a final lossless layer.

So, looks like everything is correct then. Closing.

@boxerab boxerab closed this as completed Sep 1, 2017
@rouault
Copy link
Collaborator

rouault commented Sep 1, 2017

For -r flag, 1 indicates a final lossless layer

Actually that doesn't match my experiments. You really need to set 0 to get all the remaining coding passes in the final layer. 1 gets you almost to lossless, but not completely

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

Well, what I would do is keep the 1 for compression ratio flag, and then convert to 0 in the code. Compression ratio of 0 is a bit counter-intuitive. You could let the user know that 1 == final lossless layer.

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

i.e. set rates[layer-1] to 0 if user has final ratio of 1.

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

Also sanity check that ratios are decreasing and >= 1

rouault added a commit that referenced this issue Sep 1, 2017
…alue to get lossless for -r. In opj_j2k_setup_encoder(), make sure that ll rates[] <= 1.0 are set to 0. Document 0 as being lossless for -q / tcp_distoratio (#1009)
@boxerab boxerab reopened this Sep 1, 2017
@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

So, I think the key logic to look at is how distortion delta dd and rate delta dr are handled in rate control. When dd and dr are both zero (as is the case for passes that do not contribute to rate), these passes are ignored . I guess code assumes that a subsequent layer will then force them to be included. But, if this is the final layer, and the final passes, then these passes will be lost.

So, that's why there should be no rate control at all on final layer, if it is a lossless one. Just include everything and do not look at dd or dr.

rouault added a commit that referenced this issue Sep 1, 2017
rouault added a commit that referenced this issue Sep 1, 2017
…yer (#1009)

And save a useless loop, which should be a tiny faster.
@rouault
Copy link
Collaborator

rouault commented Sep 1, 2017

OK, b428b8c should really make sure to include all passes for a lossless layer

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

Looks good.

@boxerab boxerab closed this as completed Sep 1, 2017
@rouault
Copy link
Collaborator

rouault commented Sep 1, 2017

You might want to check that only the final layer is specified as lossless by the user.

Yes, 7aa071a will warn about that

@boxerab
Copy link
Contributor Author

boxerab commented Sep 1, 2017

Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants