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

Lepton support #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SergeiVorobyov
Copy link

@SergeiVorobyov SergeiVorobyov commented Mar 25, 2023

Examples of Dropbox Lepton images can be found there: https://github.com/microsoft/lepton_jpeg_rust/tree/main/images

@@ -112,7 +112,7 @@
<DebugInformationFormat>EditAndContinue</DebugInformationFormat>
<RuntimeTypeInfo>true</RuntimeTypeInfo>
<OpenMPSupport>false</OpenMPSupport>
<LanguageStandard>stdcpp17</LanguageStandard>
<LanguageStandard>stdcpp20</LanguageStandard>
Copy link
Owner

Choose a reason for hiding this comment

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

What new language feature is used in the new code that requires cpp20?

Copy link
Author

Choose a reason for hiding this comment

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

In fact ISO C++20 Standard is not required for my changes. I can revert it back or left it as is to have latest language version support in the project.

Copy link
Author

Choose a reason for hiding this comment

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

Reverted it back,

@sylikc
Copy link
Owner

sylikc commented Mar 25, 2023

Sergei, thanks for the PR and bugfix... I need to review the way the the support has been added.

Having an external executable write to a file is not really support the way that JPEGView supports other formats, would likely interact poorly with file change detection, etc, and would be considerably slow when browsing. Since you're already hooked into ImageLoadThread, can you add in support via the lepton library?

@SergeiVorobyov
Copy link
Author

Sergei, thanks for the PR and bugfix... I need to review the way the the support has been added.

Having an external executable write to a file is not really support the way that JPEGView supports other formats, would likely interact poorly with file change detection, etc, and would be considerably slow when browsing. Since you're already hooked into ImageLoadThread, can you add in support via the lepton library?

Actually the Lepton library doesn't exist. At least I didn't see it yet. Please let me know if you know any implementation that can be used as the library. Standalone converter works with a temporary file, and it is an only area for a possible improvement. I could try to switch the temporary file exchange to pipes. It could save some time by reducing temporary JPEG file IO operation time. Fortunately, lepton-avx.exe works with pipes well.

@sylikc
Copy link
Owner

sylikc commented Mar 31, 2023

@SergeiVorobyov I've been swamped with work irl this past week, i'll take a look after this weekend. Before I left off, I was considering looking at the lepton source to extract the relevant parts... it's APL which is GPL compatible.

(Thanks for removing the updated project/solution files and squashing the commits. That makes it easier to merge.)

Piping

I could try to switch the temporary file exchange to pipes. It could save some time by reducing temporary JPEG file IO operation time.

I would like the idea of piping instead of writing to a file and reading back. I think that would at least be partially cleaner. If you can implement that piping, that'd be great

Other thoughts I had was, I would also need to test whether this executable idea works in the context of 32-bit and 64-bit, and like, that executable says AVX, whether it supports other environments, or what's the system requirements.

Generic similar support method

And lastly, if this type of file can be supported by an external executable and then piped into JPEGView, if it can be coded in a generic way which would pretty much support all types of other files with an external executable. Like I was going to investigate and look into your implementation if it can be made to support pretty much any type of file... say, via a config file, like an INI setting or like (just speaking off the top of my head) another config file to support other file types using the same method.

@sylikc sylikc added the format support Related to add/remove/change of a specific format support. label Mar 31, 2023
@sylikc sylikc added the backlog This isn't a priority add/fix at this time label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog This isn't a priority add/fix at this time format support Related to add/remove/change of a specific format support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants