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

Feature Request: Save Progressive jpeg #449

Open
nla-brandonjames opened this issue Jan 24, 2018 · 15 comments
Open

Feature Request: Save Progressive jpeg #449

nla-brandonjames opened this issue Jan 24, 2018 · 15 comments

Comments

@nla-brandonjames
Copy link

Description

I can't find an API in ImageSharp to save my images in the progressive jpeg format. It is a requirement for my application. I am capable of saving as a baseline image.

  • ImageSharp version: 1.0.0-beta2
  • Other ImageSharp packages and versions:
  • Environment (Operating system, version and so on): Windows 10 (dev) Ubuntu 16.04 (prod)
  • .NET Framework version: .NET Core 2.0.5
@JimBobSquarePants
Copy link
Member

@nla-brandonjames

Unfortunately we don't have support for saving progressive jpegs. See #10 It's something I REALLY want though.

We'd need support from someone in the community if we are in any way likely to get it for v1.0 as jpeg is.... complicated. At the moment you would have to post process the images using something like mozjpeg.

Cheers

James

P.S In the future could you please ask questions in the Gitter channel? We're trying to keep the issue tracker clear so we can stay on top of things.

@nla-brandonjames
Copy link
Author

@JimBobSquarePants Okay, will do. I have a couple more things to add to this.

@JimBobSquarePants
Copy link
Member

Thanks! 👍

@antonfirsov antonfirsov changed the title How To Save as a Progressive jpeg Feature Request: Save Progressive jpeg Jan 18, 2020
@JimBobSquarePants JimBobSquarePants added this to the Future milestone Apr 24, 2020
@maartenmensink
Copy link

Is this still on the roadmap?

@JimBobSquarePants
Copy link
Member

If someone wants to implement it yes.

@maartenmensink
Copy link

maartenmensink commented Jun 2, 2021

We are willing to help with the implementation for sure. My first gut feeling is that we would expand the current JpegEncoder with a property Progressive which should default to false.

image

Jpeg Progressive Encoder

  • Second we need to port a good working progressive encoder like libjpeg-turbo if that is offcourse allowed with respect to their license
  • Create one from scratch.

What are your considerations in regards to this?

@antonfirsov
Copy link
Member

antonfirsov commented Jun 2, 2021

port a good working progressive encoder like libjpeg-turbo

Note that libjpeg-turbo has it's own memory management primitives, a bunch of C indirections, and the optimized SIMD methods are implemented in assembly, which makes it extremely hard to port it in an efficient manner. There is a naive port of classic LibJpeg under https://github.com/BitMiracle/libjpeg.net/tree/master/LibJpeg/Classic/Internal. Such a port will leave you in a place where both the performance and the code quality is quite bad, and a huge amount of additional work has to be spent to bring it to an acceptable level.

Unless someone is really experienced with porting native code including the SIMD bits, I would prefer extending our current encoder with the bits needed for saving progressive jpegs. Significant amount of performance work has been spent on it already, including SIMD optimization of color codecs (#1508), I think it would be more beneficial to build on top of that work than to start over from scratch.

new JpegEncoder { Progressive = true }

One should be also able to set the number of scans.

@JimBobSquarePants
Copy link
Member

We are willing to help with the implementation for sure.
@maartenmensink wonderful, thank you! ❤️

Unless someone is really experienced with porting native code including the SIMD bits, I would prefer extending our current encoder

Yep agreed, this is the sanest approach. Following #1632 that should be much easier to do.

@br3aker
Copy link
Contributor

br3aker commented Jun 2, 2021

new JpegEncoder { Progressive = true }

One should be also able to set the number of scans.

It's better to provide just a ProgressiveScansCount property and store it as a nullable type, otherwise it can look silly:

new JpegEncoder { ScanCount = 10 } // looks like it is progressive but default flag is false which is ambiguous

new JpegEncoder { 
    ScanCount = 10,
    IsProgressive = true // DRY violation, practically
}

Nullable would also eliminate bool flag:

// somewhere in encoding code
encoder.Encode(/*settings*/, isProgressive: this.ScanCount.HasValue);

@maartenmensink
Copy link

maartenmensink commented Jun 2, 2021

It took inspiration from another .net based library they only provide the option of Progressive. As far as i could tell they do not provide a scans property. Only providing a property ScanCount would be weird. As it is a obscure name. You cant read the intention from the property imho.

Sorry just saw your edit. ProgressiveScansCount is a better suggestion. Still the question remains if you dont want a default for the number of scans?

@br3aker
Copy link
Contributor

br3aker commented Jun 2, 2021

Sorry just saw your edit. ProgressiveScansCount is a better suggestion. Still the question remains if you dont want a default for the number of scans?

public int? ProgressiveScanCount;

It would be null by default so implementation can check:

if(ProgressiveScanCount.HasValue)
{
    // Progressive encoding
}
else 
{
    // Baseline encoding
]

There's also a problem if you actually want a default scan count value. I've changed my mind, two properties are better:

// progressive with default count
new JpegEncoder { IsProgressive = true }
// progressive and custom count
new JpegEncoder { IsProgressive = true, ProgressiveScans = 10 }
// progressive with implicit `true` flag
new JpegEncoder { ProgressiveScans = 10 }

I don't really like the last one but I guess it's a trade-off for the first 2 setups.

@JimBobSquarePants
Copy link
Member

Would the number of scans be something we want to offer as a tweakable property? Does libjpeg (turbo) offer this?

@maartenmensink
Copy link

https://github.com/libjpeg-turbo/libjpeg-turbo/blob/main/wizard.txt#L114

-progressive switch generates a progressive JPEG file using a default seriesof progression parameters.
-scans file     Use the scan sequence given in the named file.

You can define a scan script which controls how a progressive jpeg is encoded.

@JimBobSquarePants
Copy link
Member

Ooft. Don’t like the idea of a script, far too brittle.

@antonfirsov
Copy link
Member

The semantic of that script is the most general way of defining a progressive jpeg export. We should define an equivalent internal API to have a flexible encoder implementation. This can be something like a ScanDefinition[], where ScanDefinition is a descriptor of coefficients & channels that go to a specific scan.

Another question is the API shape we want to expose to the user. Considering the size and the complexity of the topic I think it's way too early to make any conclusions on that. Someone should go and prototype the thing first. It will be much more straightforward to discuss API details when we already have a working POC implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants