lighttable: crash when zoom in on full preview of image without history #2377
Comments
|
yes tricky, sadly all my raw seems to have an embedded "full-sized" jpeg. Can you maybe share a raw file with this behavior ? (or point me to a camera which produce such raw) For the crash, you don't seem to run an up-to-date git version (the line numbers don't match) as I've pushed number of fix in this area recently, can you maybe retry with latest commits ? |
|
Thanks for looking at it! I'm seeing this with an X100F. It looks like https://raw.pixls.us/getfile.php/1942/nice/Fujifilm%20-%20X100F%20-%2014bit%20uncompressed%20(3:2).RAF is a good test candidate. For what it is worth, I'm looking fullscreen on a 4K monitor. I tested this on commit 85ccaaa, so this should be current enough. The line numbers don't match because I added some debugging printf's. If it helps I can provide a backtrace and better replication instructions. |
|
hmm, I don't manage to reproduce the crash here.
For the other point, if the zoom can't go at 100%, the feature will be useless for user with those cameras. So imho, we should compute larger mipmaps with dt pipe. Questions are :
|
|
I'm working on replicating the crash here. I can't get it to work with a clean install/config and only the raw.pixls.us RAF. I tend to only be able to trigger it by moving through a number of images then zooming in on one with no cached mip8. More replication work forthcoming! The/a problem seems to be that in the case of mip8, Using the RAF sample file, you should at least be able to see that the zoom limit on an image with no history stops at embedded JPEG resolution? And those are good questions. There already is a config option, of course: I appreciate seeing thumbnails based on the embedded JPEGs, as they have good camera-provided color/contrast and show up rapidly. I'd hate to lose that for the sake of provide 100% zoom in the rare cases I want that for a picture without a history. One possibility would be for the mip8 to flash a warning that this is based on pixelpipe recalc, but keep mip0-7 as is. It could be nice, though, to keep using the embedded JPEG through the limit of its resolution (which may be 100% for many cameras). I do worry about adding too much complexity to accommodate a corner case. Given the choice I'd rather keep mip0-7 fast (from embedded JPEG) and mip8 slow but 100% from the raw if necessary. And still give users the chance to flip |
|
I can replicate the crash with a clean config. I can see it with either fullscreen (4k) or a smaller window size (2542x810, which is minimum allowed width). A backtrace is attached. Steps to replicate:
This may be dependent a bit on the speed of performing steps 6 & 7 (maybe needs to be fast enough so that the intermediate mipmaps aren't updated). Here's a backtrace with a smaller window: And, fwiw, fullscreen: And a backtrace via gdb Thread 40 "worker 2" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff9ca67300 (LWP 1679)]
.omp_outlined._debug__ (.global_tid.=<optimized out>, out=<optimized out>, wd=<optimized out>, in=<optimized out>, iw=<optimized out>,
jj=<optimized out>, ii=<optimized out>, sj=<optimized out>, scale=<optimized out>, si=<optimized out>, offm=<optimized out>,
offM=<optimized out>, ih=<optimized out>, half_pixel=<optimized out>, .bound_tid.=<optimized out>, ht=<optimized out>, bpp=<optimized out>)
at ../src/develop/imageop_math.c:87
87 out2[k] = // in3[k];
(gdb) bt
#0 0x00007ffff7dd6b79 in .omp_outlined._debug__
(.global_tid.=<optimized out>, out=<optimized out>, wd=<optimized out>, in=<optimized out>, iw=<optimized out>, jj=<optimized out>, ii=<optimized out>, sj=<optimized out>, scale=<optimized out>, si=<optimized out>, offm=<optimized out>, offM=<optimized out>, ih=<optimized out>, half_pixel=<optimized out>, .bound_tid.=<optimized out>, ht=<optimized out>, bpp=<optimized out>) at ../src/develop/imageop_math.c:87
#1 0x00007ffff7dd6b79 in .omp_outlined.
(.global_tid.=<optimized out>, .bound_tid.=<optimized out>, ht=<optimized out>, out=<optimized out>, bpp=<optimized out>, wd=<optimized out>, in=<optimized out>, iw=<optimized out>, jj=<optimized out>, ii=<optimized out>, sj=<optimized out>, scale=<optimized out>, si=<optimized out>, offm=<optimized out>, offM=<optimized out>, ih=<optimized out>, half_pixel=<optimized out>) at ../src/develop/imageop_math.c:72
#2 0x00007ffff78c4c93 in __kmp_invoke_microtask () at /lib/x86_64-linux-gnu/libomp.so.5
#3 0x00007ffff7862dd8 in () at /lib/x86_64-linux-gnu/libomp.so.5
#4 0x00007ffff7861ac5 in () at /lib/x86_64-linux-gnu/libomp.so.5
#5 0x00007ffff78b3337 in () at /lib/x86_64-linux-gnu/libomp.so.5
#6 0x00007ffff7c54fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486
#7 0x00007ffff7b8582f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95I'm testing at 9dce2d1 as I think the second window code may have introduced some GTK troubles. |
|
ok, I can reproduce. seems that the allocated mem is not right... for mipmap_8 for the other problem, I think you're right : just compute what is needed with a message to warn the user and keep small thumbnails consistent. |
|
That sounds right that the "real" output size may actually not be what we need! Quite glad that you can reproduce it. And yes, I think users will just be grateful for the option of a 100% zoom of whatever quality is available. I'm not clear if there needs to be yet another mipmap level, one for 100% of the embedded JPEG and one for 100% of the raw... If the memory allocation problem is orthogonal to the JPEG vs. raw zoom question then there's time to figure out the latter later. |
|
ok, I've sort of a fix (by allocating the image size for unaltered images in |
|
I'm happy to try out any fix, though I may be off the computer for a bit until Monday. It does seem that darktable/src/common/mipmap_cache.c Line 1205 in 9dce2d1 |
|
ouch... seems like it's exiv2 which report wrong values... In fact, it seems that this raw store exif values of the embeded jpeg and not the raw file... I'll try to investigate further, but all this is completely new for me... index e588207..120b782 100644
--- a/src/common/mipmap_cache.c
+++ b/src/common/mipmap_cache.c
@@ -317,36 +317,50 @@ void dt_mipmap_cache_allocate_dynamic(void *data, dt_cache_entry_t *entry)
{
if(mip == DT_MIPMAP_8)
{
- // we first need to get the final output size of our image
- // let's create a dummy pipe for that
const uint32_t imgid = get_imgid(entry->key);
- const dt_image_t *img2 = dt_image_cache_get(darktable.image_cache, imgid, 'r');
- dt_image_t imgtmp = *img2;
- dt_image_cache_read_release(darktable.image_cache, img2);
- dt_develop_t dev;
- dt_dev_init(&dev, 0);
- dt_dev_load_image(&dev, imgid);
- dt_dev_pixelpipe_t pipe;
- const int res = dt_dev_pixelpipe_init_dummy(&pipe, imgtmp.width, imgtmp.height);
- if(res)
+ if (!dt_image_altered(imgid))
{
- // set mem pointer to 0, won't be used.
- dt_dev_pixelpipe_set_input(&pipe, &dev, NULL, imgtmp.width, imgtmp.height, 1.0f);
- dt_dev_pixelpipe_create_nodes(&pipe, &dev);
- dt_dev_pixelpipe_synch_all(&pipe, &dev);
- dt_dev_pixelpipe_get_dimensions(&pipe, &dev, pipe.iwidth, pipe.iheight, &pipe.processed_width,
- &pipe.processed_height);
- dt_dev_pixelpipe_cleanup(&pipe);
- // we enlarge the output size by 4 to handle rounding errors in mipmap computation
+ // the image has not been been altered, so output size = input size
+ // and the orientation doesn't matter here
+ const dt_image_t *img2 = dt_image_cache_get(darktable.image_cache, imgid, 'r');
entry->data_size
- = sizeof(struct dt_mipmap_buffer_dsc) + (pipe.processed_width + 4) * (pipe.processed_height + 4) * 4;
+ = sizeof(struct dt_mipmap_buffer_dsc) + (img2->width + 4) * (img2->height + 4) * 4;
+ printf("allocate2 %d %d x %d\n", imgid, img2->width, img2->height);
+ dt_image_cache_read_release(darktable.image_cache, img2);
}
else
{
- // for some raison pipeline didn't succeed, let's allocate huge memory
- entry->data_size = cache->buffer_size[mip];
+ // we first need to get the final output size of our image
+ // let's create a dummy pipe for that
+ const dt_image_t *img2 = dt_image_cache_get(darktable.image_cache, imgid, 'r');
+ dt_image_t imgtmp = *img2;
+ dt_image_cache_read_release(darktable.image_cache, img2);
+ dt_develop_t dev;
+ dt_dev_init(&dev, 0);
+ dt_dev_load_image(&dev, imgid);
+ dt_dev_pixelpipe_t pipe;
+ const int res = dt_dev_pixelpipe_init_dummy(&pipe, imgtmp.width, imgtmp.height);
+ if(res)
+ {
+ // set mem pointer to 0, won't be used.
+ dt_dev_pixelpipe_set_input(&pipe, &dev, NULL, imgtmp.width, imgtmp.height, 1.0f);
+ dt_dev_pixelpipe_create_nodes(&pipe, &dev);
+ dt_dev_pixelpipe_synch_all(&pipe, &dev);
+ dt_dev_pixelpipe_get_dimensions(&pipe, &dev, pipe.iwidth, pipe.iheight, &pipe.processed_width,
+ &pipe.processed_height);
+ dt_dev_pixelpipe_cleanup(&pipe);
+ // we enlarge the output size by 4 to handle rounding errors in mipmap computation
+ entry->data_size
+ = sizeof(struct dt_mipmap_buffer_dsc) + (pipe.processed_width + 4) * (pipe.processed_height + 4) * 4;
+ printf("allocate %d %d x %d\n", imgid, pipe.processed_width, pipe.processed_height);
+ }
+ else
+ {
+ // for some raison pipeline didn't succeed, let's allocate huge memory
+ entry->data_size = cache->buffer_size[mip];
+ }
+ dt_dev_cleanup(&dev);
}
- dt_dev_cleanup(&dev);
}
else if(mip <= DT_MIPMAP_F)
{It works, but it's not the right fix, as it can't be used for computing the real full-res mipmap... |
|
Yes, tricky! The fix you're looks sound to me, but I understand that it can become a litany of special cases. I think this is what people mean by "technical debt" -- there are bits of the pipeline, mipmaps, and caching which don't quite fit in with our abstract ideas about what it should be, though as long as we use it only in certain ways, things don't break. But an interesting expansion of the project such as adding mip8 suddenly exposes fissures. On more thought, I don't think it's a terrible outcome if zooming in on unaltered images only goes to the limit of the embedded JPEG. That still gives some information (a lot in the case of a full-size embedded JPEG) and no inconsistency between the JPEG and the raw via pixelpipe. A user can still jump into darkroom view to see more information. So long as it is documented that this is a known limitation and that changing |
|
Please have a look at PR #2397 |
|
Fixed as PR merged (manually). |
Describe the bug
The new zoom feature in lighttable (#2267) gets confused when zooming in on images without a history. This can result in a crash.
To Reproduce
Steps to reproduce the behavior:
dt_iop_flip_and_zoom_8.Alternately, dt will zoom in on the image, but not to the full resolution of the raw file but rather to the maximum resolution of the JPEG preview embeddded within the raw file.
This may be further complicated by the setting of
never_use_embedded_thumbconf value.I have trouble replicating this bug consistently. I think it's easiest when there are not yet entries in the cache from the image. (Try running with --cachedir set to an empty directory.)
Expected behavior
This is a tricky one. Should it zoom in on the JPEG within the raw? In this case we don't see the full resolution of the raw file. But if we run the pixelpipe when we start showing a mip8 to show all the pixels, this will not be consistent with the embedded JPEG data used to produce mip0-7.
Backtrace
Here's a backtrace with OpenMP turned off
The text was updated successfully, but these errors were encountered: