search

FLIF-hub / FLIF

Free Lossless Image Format
Other
3.72k stars 229 forks source link

Issue decode_over=true when quality is 10000 #431

Closed saschanaz closed 7 years ago

saschanaz commented 7 years ago

Fixes #411.

hrj commented 7 years ago

If decode_over = qual==10000, then the need for a separate decode_over parameter goes off, because callback can easily do the computation itself.

The decode_over parameter needs to convey more information than the qual parameter.

Currently, decode_over is true when decoding is completed but qual is not 10000. This can be used by the client to decide whether the decoded image should be rendered or not. If decoding is over there is no point in waiting for the next callback. If decoding is not over, it may decide to postpone the rendering until next callback.

saschanaz commented 7 years ago

Currently, decode_over is true when decoding is completed but qual is not 10000

I think it will still be true, in my test decoding a broken file always hits line 1406 with decode_over=true. Is there some situation that this line will be skipped?

hrj commented 7 years ago

Is there some situation that this line will be skipped?

Yes, if the file is truncated in such a way that the callback is issued from inner loop, but does not make any further progress in quality. Then no other callbacks would be issued. But that is a separate issue which needs to be addressed.

As for this PR, I agree it makes things more consistent, but it is not necessary. Client can infer the same by looking at qual parameter. If the PR is merged, it changes the API slightly, so I suggest a bump of version number, atleast a bump of minor version number.

saschanaz commented 7 years ago

Yes, if the file is truncated in such a way that the callback is issued from inner loop, but does not make any further progress in quality.

I think that was my previous concern. In reality it seems it issues double callbacks where both have the same quality value.

saschanaz commented 7 years ago

If the PR is merged, it changes the API slightly, so I suggest a bump of version number, atleast a bump of minor version number.

Should I do bump it on this PR or you will do it later?

hrj commented 7 years ago

I think that was my previous concern. In reality it seems it issues double callbacks where both have the same quality value.

Oh! Would be nice to have a reproducible test case, using something like dumpflif.

Should I do bump it on this PR or you will do it later?

I am not sure if @jonsneyers would like a version bump. It was just a suggestion from my end.

saschanaz commented 7 years ago

Oh! Would be nice to have a reproducible test case, using something like dumpflif.

433

jonsneyers commented 7 years ago

OK if I just merge this? Is the minor API change going to break anything?

hrj commented 7 years ago

@jonsneyers I guess it is okay to merge. The callback logic needs to be further fixed anyway (#433), so the version bump can happen later.