SixLabors / ImageSharp

:camera: A modern, cross-platform, 2D Graphics library for .NET
https://sixlabors.com/products/imagesharp/
Other
7.42k stars 852 forks source link

Feature Request: Save Progressive jpeg #449

Open nla-brandonjames opened 6 years ago

nla-brandonjames commented 6 years ago

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.

JimBobSquarePants commented 6 years ago

@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 commented 6 years ago

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

JimBobSquarePants commented 6 years ago

Thanks! 👍

maartenmensink commented 3 years ago

Is this still on the roadmap?

JimBobSquarePants commented 3 years ago

If someone wants to implement it yes.

maartenmensink commented 3 years ago

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

What are your considerations in regards to this?

antonfirsov commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

maartenmensink commented 3 years ago

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 commented 3 years ago

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

antonfirsov commented 3 years ago

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.