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.

Sign up for GitHub

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

Jump to bottom

mipmap : fix if embedded preview size is too small #2397

Closed
wants to merge 2 commits into from
Closed

mipmap : fix if embedded preview size is too small #2397

wants to merge 2 commits into from

Conversation

AlicVB
Copy link
Contributor

@AlicVB AlicVB commented Apr 13, 2019

this fix #2377
There's 2 problem with some raw with small embedded preview images (https://raw.pixls.us/getfile.php/1942/nice/Fujifilm%20-%20X100F%20-%2014bit%20uncompressed%20(3:2).RAF) :

  • crash if one zoom too quickly in full preview mode
  • the embedded preview is small (1920x1280) so the zoom is quite limited (real image size is 6160x4032)

In fact the main problem is that exiv2 read the embedded preview size in place of the image size... So I've added a way to run minimal rawspeed pass, just to get the real dimension of images with embedded preview.

@AlicVB AlicVB mentioned this pull request Apr 13, 2019
Closed
@AlicVB AlicVB added the bugfix pull request fixing a bug label Apr 13, 2019
@dtorop
Copy link
Contributor

dtorop commented Apr 13, 2019

This looks impressive. One quick thought is if verified_size should be a gboolean rather than an int. That seems in keeping with dt coding practices.

I want to look more carefully at the dt_imageio_open_XXX() changes, and haven't actually tested this yet. More in a bit.

@AlicVB
Copy link
Contributor Author

AlicVB commented Apr 13, 2019

This looks impressive. One quick thought is if verified_size should be a gboolean rather than an int. That seems in keeping with dt coding practices.

I've just mimic the other "boolean" present in the structure...

I want to look more carefully at the dt_imageio_open_XXX() changes, and haven't actually tested this yet. More in a bit.

This is clearly the sensible area of change (that with all the cache get/release)

@dtorop
Copy link
Contributor

dtorop commented Apr 14, 2019

I tested this a bit and it looks good. Not only does It not crash, but it behaves just as I'd expect/hope, without having to think about any subtleties.

I want to look more carefully at the dt_imageio_open_XXX() changes
...
This is clearly the sensible area of change (that with all the cache get/release)

My one regret about the dt_imageio_open() work is that I wish that each sub-image type didn't have to know separately how to handle a NULL buf, and that there's implicit behavior where opening a raw goes all the way through calculating width/height before returning on NULL buf, but not for other formats. But I imagine the only way to unwind this would be to make a separate dt_imageio_get_dims() function which was used by dt_imageio_open, and that seems both absurd and wasteful, so I see why what you did makes sense.

@dtorop
Copy link
Contributor

dtorop commented Apr 14, 2019

One question: it looks like even for mipmaps < mip8, it looks like now the pixelpipe results appear rather than embedded JPEG. Is this expected/right?

@AlicVB
Copy link
Contributor Author

AlicVB commented Apr 14, 2019

One question: it looks like even for mipmaps < mip8, it looks like now the pixelpipe results appear rather than embedded JPEG. Is this expected/right?

What is expected, is that the computed mipmap appears for all mipmap levels where the embedded thumbnail is too small. In the case of the fuji example, the embedded thumbnail should be used for mipmap <= 4 and computed after (if I remember correctly the mipmap sizes). Is that what you see ?

Sadly, I can't test anything : I've broken my system with a tentative of upgrading gtk :), so it's time to a clean reinstall of a more recent OS version. Anyway, I'm leaving for holidays tomorrow, so I'll come back to dev in 10-15 days.

@TurboGit TurboGit self-requested a review April 14, 2019 11:55
@TurboGit TurboGit self-assigned this Apr 14, 2019
@TurboGit TurboGit added this to the 2.8 milestone Apr 14, 2019
@TurboGit
Copy link
Member

@AlicVB : since you won't be around for some days I'll wait for your come back to merge this. Ping me when merging can be done. Thanks.

@AlicVB
Copy link
Contributor Author

AlicVB commented Apr 25, 2019

Ok, I've just added some refactoring around the image "final size" computation. Now final size are stored in dt_image_t and compute if needed (first need / after image changes)
As a side effect, this allow to avoid some "jumpy" effect in lighttable preview zooming when you zoom to max quickly.

Note :

  • this sizes are not stored in db, so they need to be recompute at each session. Eventually this can be changed, but this need a db schema change and to be sure to update the db on each image change (2 sensible areas I know nothing about, so I've avoid it)

TurboGit
TurboGit requested changes Apr 26, 2019
View reviewed changes
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not tested yet. Just some minor comments. Also, please make sure rawspeed is not changed here. Thanks.

src/views/lighttable.c Outdated
@@ -353,7 +353,7 @@ static inline void _destroy_preview_surface(dt_preview_surface_t *fp_surf)
fp_surf->imgid = -1;
fp_surf->w_lock = 0;

fp_surf->zoom_100 = 40.0f;
fp_surf->zoom_100 = 1001.0f; // dummy vaule to say it need recompute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: vaule -> value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/common/image.c

// the orientation for this camera is not read correctly from exiv2, so we need
// to go the full path (as the thumbnail will be flipped the wrong way round)
const int incompatible = !strncmp(img.exif_maker, "Phase One", 9);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look like refactoring. The orientation is wrong with current version? Note that I have recently fixed lot of orientation issues. Have you tested with current version? Just to be sure this is still needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea, this is copy/paste from mipmap_cache.c line 1187. Git blame says it's a commit from @hanatos 7 years ago, and it's a problem for Phase One h25 models (raw pixls didn't seems to have this raw...)

@AlicVB
Copy link
Contributor Author

AlicVB commented Apr 26, 2019

for the rawspeed files, I don't know where they come from, git show doesn't seems to have them here, certainly some subtilities of the submodules...
I'm not sure how to fix it, github doesn't want to remove them directly from the pull request... Any hint ?
Thank you... (and apologize if I've done something stupid...)

@TurboGit
Copy link
Member

Working fine on my side.
I have merged this manually to remove the rawspeed change.

Thanks.

@TurboGit TurboGit closed this Apr 27, 2019
@AlicVB AlicVB deleted the embedded_prev_fix branch May 4, 2019 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TurboGit TurboGit TurboGit requested changes

Assignees

@TurboGit TurboGit

Labels
bugfix pull request fixing a bug
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

lighttable: crash when zoom in on full preview of image without history
3 participants
@AlicVB @dtorop @TurboGit