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

JPEG: Support panoramas taken with a Samsung S21 #3363

Closed
maxime1992 opened this issue May 3, 2023 · 7 comments
Closed

JPEG: Support panoramas taken with a Samsung S21 #3363

maxime1992 opened this issue May 3, 2023 · 7 comments
Assignees
Labels
enhancement Refactoring, improvement or maintenance task released Available in the stable release

Comments

@maxime1992
Copy link

1. What is not working as documented?

Latest release includes improvement for broken pictures.

But all the panoramas taken with a Samsung S21 are still appearing as "upload successful" while they're actually not.

2. How can we reproduce it?

Steps to reproduce the behavior:

Download this image (just as a sanity check in case github does any kind of processing, the md5 checksum should be 75171ad4f47a883b97e73bced03a29c7):

20220605_153254

  • Try to upload it on Photoprism
  • At the end, notice that whether you get a success or an error for the upload, the file won't be in your gallery

3. What behavior do you expect?

Being able to upload my panorama successfully

4. What could be the cause of your problem?

Looking at related issues #1944, #1407, #2463 and this comment in particular #1407 (comment), I suspect the issue is in the golang repo mentionned. BUT, there seem to be a 1 line fix when consuming the picture and I'm hoping it can be as simple as this on Photoprism side to fix this for now and eventually remove that useless 1 line fix once it's fixed upstream. But the upstream issue has been open for a while and it'd be lovely to have that fix available in Photoprism meanwhile. I know it's not ideal, but if it's really a 1 line fix, it'd be tremendously helpful 🙏

And for refence, I'm creating this new issue as suggested here.

5. Can you provide us with example files for testing, error logs, or screenshots?

Testing file uploaded above.

The kind of logs I'm getting is this:

import: failed creating thumbnails for 2021/11/20211128_141554_7B99F85D.jpg (EOF while decoding)

6. Which software versions do you use?

(a) PhotoPrism Architecture & Build Number: AMD64, ARM64, ARMv7,...

AMD64, build 230502-c405f6eff

(b) Database Type & Version: MariaDB, MySQL, SQLite,...

MariaDB 10.11 as suggested in the release note

(c) Operating System Types & Versions: Linux, Windows, Android,...

Linux / official Photoprism docker image

(d) Browser Types & Versions: Firefox, Chrome, Safari on iPhone,...

Chrome

(e) Ad Blockers, Browser Plugins, and/or Firewall Software?

Yes but works fine for any other picture

@maxime1992 maxime1992 added the bug Something isn't working label May 3, 2023
@lastzero
Copy link
Member

lastzero commented May 5, 2023

Yeah, the problem is probably that the upload is indeed successful, but the indexing fails. We don't yet know if all files can be indexed properly with all metadata, etc. at the time the success message is displayed. However, you should find error or warning logs under Library > Errors?

@lastzero
Copy link
Member

lastzero commented May 5, 2023

What would be a good solution (apart from never having problems with indexing)? Show another message that says "Sorry, upload was not successful, ignore the previous information"?

@lastzero lastzero changed the title Panoramas "upload successful" but actually there's an error and it's not imported UX: Warn user if upload was successful but indexing failed May 5, 2023
@lastzero lastzero added enhancement Refactoring, improvement or maintenance task ux Impacts User Experience and removed bug Something isn't working labels May 5, 2023
@lastzero lastzero self-assigned this May 5, 2023
@lastzero
Copy link
Member

lastzero commented May 5, 2023

Of course, we will also take a look at improving the JPEG reader, but due to the many other tasks, I can't promise a quick solution. Ideally a contributor can help with this! Thank you :)

@lastzero lastzero added the help wanted Well suited for external contributors! label May 5, 2023
@lastzero lastzero removed their assignment May 5, 2023
@maxime1992 maxime1992 changed the title UX: Warn user if upload was successful but indexing failed Broken file and failed creating thumbnails after upload May 5, 2023
@maxime1992 maxime1992 changed the title Broken file and failed creating thumbnails after upload Broken file leads to an error when creating thumbnails after upload and the image isn't visible May 5, 2023
@maxime1992
Copy link
Author

maxime1992 commented May 5, 2023

@lastzero I've seen you've changed the title to "UX: Warn user if upload was successful but indexing failed 1".
I've edited that and changed it to "Broken file leads to an error when creating thumbnails after upload and the image isn't visible" as this is the issue I'm really after here.

That said, I did open a separate issue to track that specific problem here: #3371. Can you edit the labels accordingly please as this issue isn't about UX.

Yeah, the problem is probably that the upload is indeed successful, but the indexing fails. We don't yet know if all files can be indexed properly with all metadata, etc. at the time the success message is displayed

Yup that'll be part of the new issue I've created to track the UX issue but it's not what I'm after here. In that very specific case, I'd just like not to have a crash in the first place and being able to upload my panoramas without running a bunch of commands with image magick to fix them upstream. What I'm after would be to have this fix included so that the bad images are not crashing.

However, you should find error or warning logs under Library > Errors?

Correct. But because it appears as successful, I'm not tempted by any mean to open the logs. I only did because by chance I noticed that the album had a different number of pictures than what I had originally. But let's move UX discussion to the new issue.

What would be a good solution (apart from never having problems with indexing)? Show another message that says "Sorry, upload was not successful, ignore the previous information"?

I'll reference that comment in the new issue and answer here.

Of course, we will also take a look at improving the JPEG reader

Awesome 🔥. Hopefully it's limited to this little fix 🤞

due to the many other tasks, I can't promise a quick solution

I understand. While I'm not in a position to help, I'm a developer and I know what a backlog is like 🙃. If you need any more info about this or if you've got any issue to repro, do ping me. Happy to help out.

Last but not least, as I see this issue already have a few ":+1:", in case someone is interested in a temporary fix to go pretty much automatically, through a lot of pics, I've explained how to do that here.

@lastzero lastzero added bug Something isn't working and removed enhancement Refactoring, improvement or maintenance task ux Impacts User Experience labels May 5, 2023
@lastzero lastzero changed the title Broken file leads to an error when creating thumbnails after upload and the image isn't visible JPEG: Support panoramas taken with a Samsung S21 May 5, 2023
@lastzero lastzero added enhancement Refactoring, improvement or maintenance task and removed bug Something isn't working labels May 5, 2023
@lastzero lastzero moved this to Ideas 💭 in Roadmap 🚀✨ Jun 8, 2023
@graciousgrey graciousgrey moved this from Ideas 💭 to Preview 🐳 in Roadmap 🚀✨ Jun 29, 2023
@graciousgrey graciousgrey added the please-test Ready for acceptance test label Jun 29, 2023
@graciousgrey
Copy link
Member

This should be fixed with

However, we should take a look at the indexing performance, because it seems slower now, at least for files with errors.

lastzero added a commit that referenced this issue Jun 29, 2023
Signed-off-by: Michael Mayer <michael@photoprism.app>
@maxime1992
Copy link
Author

I just tried the preview version and I can confirm with my test image above that this is now fixed! 🎉

Thanks a lot

@lastzero lastzero self-assigned this Jun 30, 2023
@lastzero lastzero added tested Changes have been tested successfully and removed please-test Ready for acceptance test help wanted Well suited for external contributors! labels Jun 30, 2023
@lastzero lastzero moved this from Preview 🐳 to Released 🌈 in Roadmap 🚀✨ Jul 19, 2023
@lastzero lastzero added released Available in the stable release and removed tested Changes have been tested successfully labels Jul 19, 2023
@lastzero
Copy link
Member

@maxime1992 While the changes did not help to read the "broken" JPEGs without re-encoding them, they broke the repair mechanism that we implemented for Samsung panoramas. I've just added (yet another) fix for this, see:

An updated preview build will be available for testing soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Refactoring, improvement or maintenance task released Available in the stable release
Projects
Status: Release 🌈
Development

No branches or pull requests

3 participants