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

ARW: Support new LJPEG compression on ILCE-7M4 #386

Closed
wants to merge 3 commits into from

Conversation

artemist
Copy link
Contributor

@artemist artemist commented Oct 3, 2022

This is a rather hacky implementation of the LJPEG lossless RAW format on the Sony ILCE-7M4 and ILCE-1. I haven't been able to test with LJPEG DNGs which do not use the interleaved rows but it should work. This does not seem to break support for lossy or uncompressed ARWs in my tests

src/librawspeed/decoders/ArwDecoder.cpp Outdated Show resolved Hide resolved
Comment on lines +332 to +341
const TiffEntry* origin_entry = raw->getEntry(TiffTag::DEFAULTCROPORIGIN);
const TiffEntry* size_entry = raw->getEntry(TiffTag::DEFAULTCROPSIZE);
iRectangle2D crop(origin_entry->getU32(0), origin_entry->getU32(1),
size_entry->getU32(0), size_entry->getU32(1));
mRaw->subFrame(crop);
Copy link
Contributor

@kmilos kmilos Oct 26, 2022

Choose a reason for hiding this comment

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

I don't think we use the file crop in any other modes (uncompressed and lossy), but rather leave it to the settings from XML? The way it is suggested now, there will be a difference between the modes, in both final image size, and the location of top left pixel...

There is a small problem to handle there, as width/height for lossless is wrongly specified by Sony as integer number of tiles, and it didn't have to be... One easy way to handle this was to have always an absolute positive crop in the XML for all modes, but it was rejected...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove it then the original under history has black bars on the bottom right, although it is cropped out later in the default module history.

Copy link
Contributor

@kmilos kmilos Feb 17, 2023

Choose a reason for hiding this comment

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

Sure, I didn't mean remove the crop completely. But we'll have to find a way to provide the same crop as uncompressed/lossy mode (defined in data/cameras.xml), and that works for APC-C/S35 files as well.

Copy link
Contributor

@kmilos kmilos Mar 30, 2023

Choose a reason for hiding this comment

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

@artemist I think this tag has the information of the maximum available raw image size that you actually want, e.g. for the 7M4:

Exif.SubImage1.0x7038                          2  7032 4688

@LebedevRI It looks like cameras.xml crop for uncompressed/lossy could be too conservative and we could have it match this by changing to "-8" (will check for other recent Sonys as well; alternatively we could start recognizing ARW 4.0 files and go directly to this tag in all modes edit: tag not available in other modes)?

Copy link
Member

Choose a reason for hiding this comment

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

If I remove it then the original under history has black bars on the bottom right, although it is cropped out later in the default module history.

This is the correct behaviour.

<...>

Since we don't use the camera-specified crop,
i don't really see why we care what the EXIF says,
we are just going to re-use the existing cameras.xml-specified crop in that mode too,
unless we need a different one, like some Fuji's need.

Copy link
Contributor

@kmilos kmilos Mar 30, 2023

Choose a reason for hiding this comment

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

i don't really see why we care what the EXIF says,

Because of this new lossless mode and tile size rounding up nonsense. This is actually the one thing that correlates it to the traditional modes and cameras.xml (unless you want to bloat cameras.xml with more entries, which I don't think is a good idea).

I'll provide an example if it's not clear.

Copy link
Contributor

@kmilos kmilos Mar 31, 2023

Choose a reason for hiding this comment

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

@LebedevRI Ok, let's take the 7M4 here - in the traditional uncompressed and lossy modes it has

Exif.SubImage1.ImageWidth                     7040
Exif.SubImage1.ImageLength                    4688

and in (tweaked) cameras.xml we put <Crop x="0" y="0" width="-8" height="0"/>, so the resulting raw image size is 7032 4688.

In the new lossless mode it has this nonsensical round up to integer multiple of 512x512 tiles:

Exif.SubImage1.ImageWidth                     7168
Exif.SubImage1.ImageLength                    5120

that produces huge black padding on the right and at the bottom, and then this new tag

Exif.SubImage1.0x7038                          7032 4688

See the pattern here?

So options for the lossless mode implementation to give us the same output size as uncompressed are 1) bloat cameras.xml with more entries just so we can have different crop numbers (and not just one; we'll need extra for APS-C outputs as remainder/padding after rounding up is different of course) or 2) just use the bloody 0x7038 tag as is because it is exactly what it means - maximum usable raw area, not "camera-specified crop".

I have verified all the ILCE-1/7M4/7SM3/7RM5 samples on RPU (dump using LibRaw's unprocessed_raw which also works for lossless) and everything matches up (w/ minor tweaks), and it also lines up perfectly for the APS-C mode of 7RM5 and cameras.xml crop for the corresponding uncompressed.

i don't really see why we care what the EXIF says.

And a final thing - this is not "crop according to EXIF", as we have just established. That would in fact be further cropping according to

Exif.SubImage1.DefaultCropOrigin              12 8
Exif.SubImage1.DefaultCropSize                7008 4672

which is what @artemist is doing now (and matching OOC JPEG size). I agree, this is not what we generally want or care about.

@kmilos
Copy link
Contributor

kmilos commented Oct 27, 2022

In general: while this might provide a temporary solution to Sony shooters, rather than making this "sonyArrange" specific, the ideal solution would be to further generalize the LJpegDecompressor class to handle MxN channel arrangement (as noted here) with M and N calculated from the TIFF tile geometry and LJPEG SOF3 header, e.g.:

1x2: conventional DNG
2x2: Sony
2x0.5: Blackmagic

@starsforeveryone
Copy link

It would be very nice to have at least a temporary fix, until the aforementioned ideal solution can be implemented. That way, darktable could add support for the lossless compressed RAW format, maybe even for this year's christmas release.

@artemist
Copy link
Contributor Author

I actually have an ILCE-7M4 now so I have a reason to get this working. I'll port this patch to the current rawspeed develop (or at least as close as possible while still building on git darktable). Maybe I'll try to deal with other channel rearrangement in another branch but that's uncertain.

@starsforeveryone
Copy link

starsforeveryone commented Feb 15, 2023

The council of A7 IV users thanks you for your service!

You just saved many hard drives' lifes.

@artemist
Copy link
Contributor Author

Okay, this commit builds on current darktable and is close enough but develop won't build with current darktable so I can't remove all the conflicts. The code isn't super clean, I need to go over it again.

@artemist
Copy link
Contributor Author

In general: while this might provide a temporary solution to Sony shooters, rather than making this "sonyArrange" specific, the ideal solution would be to further generalize the LJpegDecompressor class to handle MxN channel arrangement (as noted here) with M and N calculated from the TIFF tile geometry and LJPEG SOF3 header, e.g.:

1x2: conventional DNG 2x2: Sony 2x0.5: Blackmagic

I've been trying to figure out how to read this data from the SOF3 header. If I'm reading section A.3.3 of the very confusingly written JPEG specification correctly then that should be parsed in AbstractLJpegDecompressor::parseSOF. However, I'm getting h=1 and v=1 in a compressed JPEG.

Also, I don't understand what you mean by "2x0.5".
If you're following the JPEG specification way of notating this, Sony would be H=2, V=2.
The DNG specification doesn't say but I'm guessing it's H=2, V=1.
The only other thing that makes sense for Blackmagic is H=1, V=2.

If I can't find any other way to get H and V from the JFIF then it won't be possible to tell apart Sony and Blackmagic ordering from just the ljpeg and would have to pass it in from the raw decoder. I can definitely make my code more generic though.

@artemist
Copy link
Contributor Author

Okay, there's a better explanation of it in #334. It should work with blackmagic ordering but I can't test as the files use predictor 6 which is unsupported.

@emko
Copy link

emko commented Mar 21, 2023

Okay, this commit builds on current darktable and is close enough but develop won't build with current darktable so I can't remove all the conflicts. The code isn't super clean, I need to go over it again

is it still build-able for you using current darktable?

@artemist
Copy link
Contributor Author

It looks like they updated the submodule so this won't build, I'll work on a cleaner rewrite on develop when I have time

@da-phil
Copy link
Contributor

da-phil commented Mar 26, 2023

It looks like they updated the submodule so this won't build, I'll work on a cleaner rewrite on develop when I have time

This would be super awesome, as I already gave it a quick try to rebase against current develop, which is not so easy after so much recent refactoring work, then I run out of time 😅

@LebedevRI
Copy link
Member

I wanted to tackle this, by first tackling the more well-defined part of the problem:
this is extending LJpeg decoder and is adding a new user for it.
Effectively, i'm somewhat vary of assuming that whatever Sony produced
is in the canonical format.

I wanted to try running the sample in question through ADC,
in hopes that it would preserve the "weird" LJpeg frame vs image scale,
but it seems like nothing has changed and wine-devel still does not support modern ADC...

Is anyone aware of some LJpeg DNG's that have similar square 2x2 LJpeg MCU,
and are still using predictor=0?

@cryptomilk
Copy link
Contributor

ADC works fine for me with wine. Does some functionality not work or in general?

@kmilos
Copy link
Contributor

kmilos commented Apr 3, 2023

Is anyone aware of some LJpeg DNG's that have similar square 2x2 LJpeg MCU,
and are still using predictor=0?

I'm not, sorry. @cytrinox could possibly construct some, but not sure if that's what you're after.

Happy to run ADC for you though (they even bumped more Windows requirements for recent versions) and share on gdrive if that's useful?

@kmilos
Copy link
Contributor

kmilos commented Apr 3, 2023

@LebedevRI Please find attached the exiftool output for both the original lossless ARW and DNG from ILCE-7M4 for starters:

Sony - ILCE-7M4 - 14bit (4_3).ARW.txt
Sony - ILCE-7M4 - 14bit (4_3).dng.txt

@LebedevRI
Copy link
Member

ADC works fine for me with wine. Does some functionality not work or in general?

Hm. For me it (the newest version) simply fails to install with "this windows version is not supported",
it wants win11 or newest win10, and wine does not even support mimicking win11,
and i tried editing the register, did not help either. Maybe i should try rm -rf .wine though.

Happy to run ADC for you though (they even bumped more Windows requirements for recent versions) and share on gdrive if that's useful?

Yes, please, thank you!
Could you please just zip the Sony - ILCE-7M4 - 14bit (4_3).dng and upload it here?
It seems like it did end up with LJpeg, but exif output obviously does not parse the LJpeg itself,
so i can not tell what it's frame size is.

@kmilos
Copy link
Contributor

kmilos commented Apr 3, 2023

Yes, please, thank you!
Could you please just zip the Sony - ILCE-7M4 - 14bit (4_3).dng and upload it here?

Shoot, above the 25MB limit... Unfortunately we don' have a smaller res lossless 7SM3 instead on RPU (was added w/ later firmware).

Edit: gotta love 7-Zip, there is an option to split! (You'll have to reverse the extension back to .zip.001 because of GH limitations as well.)

Sony - ILCE-7M4 - 14bit (4_3).001.zip

@LebedevRI
Copy link
Member

LebedevRI commented Apr 3, 2023

Yes, please, thank you!
Could you please just zip the Sony - ILCE-7M4 - 14bit (4_3).dng and upload it here?

Shoot, above the 25MB limit... Unfortunately we don' have a smaller res lossless 7SM3 instead on RPU (was added w/ later firmware).

Right. Then the usual abuse - drop it onto RPU :) (without zip)

@kmilos
Copy link
Contributor

kmilos commented Apr 3, 2023

Part 2 (switch extension to .zip.002):

Sony - ILCE-7M4 - 14bit (4_3).002.zip

And obviously, this re-encodes it as Adobe's flavour of LJpeg.

@LebedevRI
Copy link
Member

And obviously, this re-encodes it as Adobe's flavour of LJpeg.

Welp, thanks for trying :)

@koolfy
Copy link

koolfy commented Apr 14, 2023

Hello, I have been watching work on this for around a year or so now in the shadows, not daring to intervene :)

I was recently trying to build a "develop" darktable with the current state of this PR merged into rawspeed.

Of course recent darktables don't compile with the head of this PR anymore, and there are a bunch of conflicts (even file renames) from this head to rawspeed head

After 3 failed attempts at managing merge conflicts, I just gave up on producing a result that compiles :)
Looks like these conflicts require actual understanding of the code to be addressed.

I understand that there are still important changes to be made before this PR is acceptable for merge into rawspeed, but do you think it would be much work for one of you guys to at least make it merge-able into "develop" for people like me who just want to produce a working darktable build with sony lossless support (functional at least for ICLE-7m4)?

I swear if I had the skill and knowledge, I would contribute somehow 😅

@da-phil
Copy link
Contributor

da-phil commented Apr 15, 2023

@koolfy I already was at the same point of trying to resolve the merge conflicts, but without properly understanding rawspeed it is quite hard, I gave up due to a lack of time.
And I think the lack of time of the other involved people is holding back this PR.
I also wish there was something which could accelerate this feature as more and more compressed sony raw files are piling up on my hard-drive and of course they want to be processed via darktable.

@da-phil
Copy link
Contributor

da-phil commented Jun 4, 2023

Hey guys (@emko, @koolfy), I just rebased this PR against latest develop and fixed the merge conflicts in PR #482. Please have a look again.
I'll do a few tests in the next few days and follow up with the above discussion with @kmilos & @LebedevRI
I also think the LJPEG code needs to be refactored and made more generic as I see a lot of code duplication between this ARW, CR2 and DNG decoder, all utilizing the LJpegDecoder or the AbstractLJpegDecoder.

@emko
Copy link

emko commented Jun 5, 2023

Hey guys (@emko, @koolfy), I just rebased this PR against latest develop and fixed the merge conflicts in PR #482. Please have a look again. I'll do a few tests in the next few days and follow up with the above discussion with @kmilos & @LebedevRI I also think the LJPEG code needs to be refactored and made more generic as I see a lot of code duplication between this ARW, CR2 and DNG decoder, all utilizing the LJpegDecoder or the AbstractLJpegDecoder.

i get all that but if the code works and it seems like no one has the time to refactor it is it really that bad? at least people can load their images

@da-phil
Copy link
Contributor

da-phil commented Jun 5, 2023

Hey guys (@emko, @koolfy), I just rebased this PR against latest develop and fixed the merge conflicts in PR #482. Please have a look again. I'll do a few tests in the next few days and follow up with the above discussion with @kmilos & @LebedevRI I also think the LJPEG code needs to be refactored and made more generic as I see a lot of code duplication between this ARW, CR2 and DNG decoder, all utilizing the LJpegDecoder or the AbstractLJpegDecoder.

i get all that but if the code works and it seems like no one has the time to refactor it is it really that bad? at least people can load their images

After understanding this mess of decoding different kind of LJPG files I think technical debt with code duplication currently is not the biggest problem, but making sure that we can decode not only compressed L files, but also the smaller M and S files, which currently does not work, see #482 (comment)
IMHO it's difficult to explain that to rawspeed / darktable users, that their compressed L files can be decoded, but not their smaller M & S files.

@LebedevRI
Copy link
Member

Thank you!
Merged in c38746d.

@LebedevRI LebedevRI closed this Jun 8, 2023
@LebedevRI
Copy link
Member

@artemist thank you for your contribution. I'm sorry that there was this much of a delay
for this particular patch, it really should not have.
The original approach of interleaveRows bool looked more complicated/scary than it was.

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.

8 participants