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

Fix .pdf gotcha #227

Closed
wants to merge 1 commit into from
Closed

Fix .pdf gotcha #227

wants to merge 1 commit into from

Conversation

za3k
Copy link
Contributor

@za3k za3k commented Sep 7, 2022

PDF does not inherently encode a resolution--it's essentially a vector format. By default, ImageMagick samples it at a tiny 72dpi resolution, so many QR codes are not read.

Default .pdf to 300dpi sample rate to fix the issue.

static int scan_image(const char *filename)
{
if (exit_code == 3)
return (-1);

int found = 0;
MagickWand *images = NewMagickWand();
if (file_extension(filename, ".pdf")) MagickSetResolution(images, 300, 300);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe replace the file_extension check with one based on MagickGetImageFormat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very sensible suggestion, but SetResolution only works before the images are loaded, and GetImageFormat only works after the images are loaded.

I'm open to suggestions on how to do it anyway--I'm pretty much an ImageMagick newbie. Seems like it would be nice to have it work for stdin, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(And before you ask, if you just unconditionally set the resolution, it overrides the DPI settings in file formats that actually do set it internally)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think you use MagickPingImage to get the format before doing the full load.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe need to remove the image created by ping image using something like MagickRemoveImage before the full load.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it would be nice to have it work for stdin, for example.

For stdin I think you could use MagickReadImageBlob and MagickPingImageBlob for reading/pinging a stdin buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change for a single image. For stdin I don't think this is possible, because you can't seek back to the beginning of stdin--once it's consumed, it's consumed, in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

For stdin I don't think this is possible, because you can't seek back to the beginning of stdin--once it's consumed, it's consumed, in general.

I think you would read stdin until EOF into a dynamically allocated char buffer then pass the buffer to imagemagic using the blob read functions(which shouldn't remove that buffer and thus allow reuse).

Anyways that's mostly separate from this fix at least but may be a nice follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, scan_image is used for "-" (which I realized as I started said followup). Currently this PR breaks all zbarimg use with stdin. I'll go ahead and figure out a fix. I'll also look into whether there's a missing test--make check shows no issues.

zbarimg/zbarimg.c Outdated Show resolved Hide resolved
zbarimg/zbarimg.c Outdated Show resolved Hide resolved
zbarimg/zbarimg.c Outdated Show resolved Hide resolved
zbarimg/zbarimg.c Outdated Show resolved Hide resolved
@za3k
Copy link
Contributor Author

za3k commented Sep 9, 2022

https://stackoverflow.com/questions/73656471/use-filetoblob-in-imagemagick well i'm a bit stuck using FileToBlob() if you can offer any help.

@jameshilliard
Copy link
Contributor

https://stackoverflow.com/questions/73656471/use-filetoblob-in-imagemagick well i'm a bit stuck using FileToBlob() if you can offer any help.

Maybe should be something like this:

void *blob = FileToBlob(filename, MagickMaxBufferExtent, &blob_length, &blob_exception);

@jameshilliard
Copy link
Contributor

or maybe:

void *blob = FileToBlob(filename, MAGICK_SSIZE_MAX, &blob_length, &blob_exception);

@za3k
Copy link
Contributor Author

za3k commented Sep 9, 2022 via email

@za3k
Copy link
Contributor Author

za3k commented Sep 9, 2022

Proposed an ImageMagick feature to help us out. If someone has thoughts on a better way to do it, feel free to comment. ImageMagick/ImageMagick#5554

Also, went ahead and reported my issues as a bug at ImageMagick/ImageMagick#5553. I can get it to work for an 80K file but not a 100K file (note size is like 100MB), which makes me think MAYBE it's not just a me issue.

In the meantime, I've made sure this doesn't break stdin, and I think it could be merged. It just won't work for PDFs on stdin yet.

@za3k
Copy link
Contributor Author

za3k commented Sep 10, 2022

Confirmed the issue I was running into was a bug. ImageMagick has a patch, thanks ImageMagick!

I'll offer a PR that makes PDFs work on stdin too, once imagemagick's stable release includes their fix.

@za3k
Copy link
Contributor Author

za3k commented Sep 11, 2022

OK, ready for submission. Apparently I was wildly wrong about how setting the density works, and I can just do it unconditionally. This is now a one-line change.

@za3k
Copy link
Contributor Author

za3k commented Sep 19, 2022

Ping on this? It's a one-line change now so it seems pretty safe. You could change the default resolution to 300 or 600 if you prefer.

@mchehab
Copy link
Owner

mchehab commented Jan 9, 2024

Merged, thanks!

@mchehab mchehab closed this Jan 9, 2024
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

Successfully merging this pull request may close these issues.

3 participants