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 and ILCE-7R5 #482

Closed
wants to merge 4 commits into from

Conversation

da-phil
Copy link
Contributor

@da-phil da-phil commented Jun 4, 2023

This PR continues where #386 stopped, so all the work in the first commit has been done by @artemist, I just rebased her changes against recent develop and fixed the merge conflicts before starting to add more work on top.

I'm still not sure about the cropping, however could only verify that it is reasonable, even though a few valid border pixels get cropped away, compared to Sony A7R5 files, which are converted to DNG. This really needs a little bit more attention before we settle on something too early.

@LebedevRI
Copy link
Member

Thank you for rebasing this!

This PR continues where #386 stopped, so all the work has been done by @artemist, I just rebased her changes against recent develop and fixed the merge conflicts.

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors might be relevant here.

<...>

I tried to look in this general direction, and the main stopping point for me is the said boolean interleaveRows.
What i have failed to understand from ITU-T81, is how we are supposed to derive the actual MCU size,
from the LJpeg itself?

@da-phil da-phil force-pushed the sony-ljpeg2 branch 4 times, most recently from 700d4d1 to ee36657 Compare June 4, 2023 22:17
* Support large-resolution ARW from ILCE-7RM5
* Generalize sonyArrange in LJPEG arrangement to support 1x2

Co-authored-by: Artemis Tosini <[email protected]>
@codecov
Copy link

codecov bot commented Jun 4, 2023

Codecov Report

Patch coverage: 25.88% and project coverage change: -0.25 ⚠️

Comparison is base (820a907) 59.06% compared to head (5f3fe8f) 58.82%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #482      +/-   ##
===========================================
- Coverage    59.06%   58.82%   -0.25%     
===========================================
  Files          232      232              
  Lines        13813    13881      +68     
  Branches      1932     1942      +10     
===========================================
+ Hits          8159     8165       +6     
- Misses        5521     5582      +61     
- Partials       133      134       +1     
Flag Coverage Δ
benchmarks 8.26% <0.00%> (-0.05%) ⬇️
integration 47.18% <27.84%> (-0.19%) ⬇️
linux 56.64% <27.50%> (-0.23%) ⬇️
macOS 18.77% <0.00%> (-0.10%) ⬇️
rpu_u 47.18% <27.84%> (-0.19%) ⬇️
unittests 18.20% <0.00%> (-0.10%) ⬇️
windows ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/librawspeed/decoders/ArwDecoder.h 60.00% <ø> (ø)
src/librawspeed/decoders/ArwDecoder.cpp 39.76% <8.77%> (-6.79%) ⬇️
...rc/librawspeed/decompressors/LJpegDecompressor.cpp 51.38% <52.17%> (-3.23%) ⬇️
src/librawspeed/decompressors/LJpegDecoder.cpp 64.28% <100.00%> (+1.32%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kmilos
Copy link
Contributor

kmilos commented Jun 5, 2023

What i have failed to understand from ITU-T81, is how we are supposed to derive the actual MCU size,
from the LJpeg itself?

You won't find this anywhere, as this is AFAICT not the subject of ITU-T81. The only way is to relate the TIFF tile size and SOF3 header yourself, as I have mentioned already: #334 (comment)

How you arrange the channels is up to you, you are in charge of the TIFF container abstraction, not ITU-T81 - SOF3 here (restricting to DNG & ARW) just provides simple, non-interleaved multi-component scans (see section 4.8.1 - it doesn't know or care where components A, B, and C came from). Hence the different schemes concocted by Adobe (Ncomp=2), Blackmagic (Ncomp=1) and Sony (Ncomp=4).

Furthermore, section 4.8.2 says this for the non-interleaved multi-component scan:

For example, in Figure 12 the MCU for the noninterleaved case is a single data unit.

meaning - a single pixel.

So, let's stop confusing the ITU-T81 interleaving w/ CFA interleaving please, which is on a higher (and independent) level of abstraction.

@da-phil
Copy link
Contributor Author

da-phil commented Jun 5, 2023

Besides the ITU-T81 discussion I just noticed that the current decoding approach fails with M and S sized compressed raws from my Sony A7r5, see images. Decoding those (M & S) files gives me the following exception:
rawspeed::ArwDecoder::DecodeLJpeg(const rawspeed::TiffIFD*) const, line 292: Unexpected bits per pixel: 15

However, it works with the files which were uploaded to raw.pixls.us: https://raw.pixls.us/data/Sony/ILCE-7RM5

It is possible that the LJPEG coding depends on some other settings (other than size), which apparently I set on my camera?

@kmilos
Copy link
Contributor

kmilos commented Jun 5, 2023

M & S ARW images are a different can of worms - they are not CFA raws, but demosaiced YCbCr...

Comment on lines 346 to 347
const TiffEntry* origin_entry = raw->getEntry(TiffTag::DEFAULTCROPORIGIN);
const TiffEntry* size_entry = raw->getEntry(TiffTag::DEFAULTCROPSIZE);
Copy link
Contributor

@kmilos kmilos Jun 5, 2023

Choose a reason for hiding this comment

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

As I already mentioned, let's not use these but the dimensions in the new tag 0x7038. We then need a new mode that'll tell us to ignore any further cameras.xml crop.

You could even go a step further and use those directly as width and height on L281-L282 without needing to do anything extra here. The LJpeg decoder should take of the padding just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to work, I just pushed the changes!

You could even go a step further and use those directly as width and height on L281-L282 without needing to do anything extra here. The LJpeg decoder should take of the padding just fine.

This however does not work, as the width & height need to be multiples of the tile size to make the tiling code work, or how did you mean that?

Copy link
Contributor

@kmilos kmilos Jun 5, 2023

Choose a reason for hiding this comment

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

They don't. The tiling code rounds up to the required multiple.

Well, at least it does in the DNG decoder, don't know if it'll just work out here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We then need a new mode that'll tell us to ignore any further cameras.xml crop.

Can we make the mandatory cropping step according to the values in cameras.xml optional by just skipping it on a decoder basis?
But I guess it would be cleaner and more flexible to introduce a new flag in cameras.xml a'la
<Crop x="0" y="0" width="0" height="0" read_from_exif="true"/>
If cropping based on exif tags fails cropping shall fall back to values provided in cameras.xml .

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, let's not call this "Exif crop". This is a pure kludge on Sony's part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will have a look later, when I've got time to finish this work package for final review ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked all the rawspeed entrypoints and decodeRaw() always comes before decodeMetaData() (both calling their inherited decoder specific function), so this should work. Will give it a try now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by the way, by resetting the history on one of my test images using the Exif.SubImage1.0x7038 crop I saw that this gives an area which still contains garbage from the tiling operation on the right and a black bar on the bottom
Screenshot from 2023-06-06 22-53-40

If I set the offset to 0,0 I only get valid pixels and no borders, so this way you can get the max. possible pixel area it seems. What's weird though is the size from Exif.SubImage1.0x7038 = 9568, 6376, but darktable crop module shows me 9567, 6376.
The DNG converted file shows 9504, 6336 in the crop module 😅

Then I did another test with way bigger crop to see where the garbage & borders begins and this proves that the values in Exif.SubImage1.0x7038 seem to be quite accurate...
Screenshot from 2023-06-06 23-51-22

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than resetting for validation, I'd remove the old images from the database (and their xmp sidecars), and import again.

The 0x7038 tag values, as mentioned, should result in the same size as the uncompressed/lossy case w/ cameras.xml crop. For 7RM5 this is, for uncompressed/lossy (9600-32)x(6376-0) which matches ljpeg compressed 0x7038 values 9568x6376.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, then the current state (of cropping) in this PR makes sense, right?

@da-phil
Copy link
Contributor Author

da-phil commented Jun 5, 2023

M & S ARW images are a different can of worms - they are not CFA raws, but demosaiced YCbCr...

OMG, why would you do that? this is quite f*cked up 😅

Answering myself to

However, it works with the files which were uploaded to raw.pixls.us: https://raw.pixls.us/data/Sony/ILCE-7RM5

it seems I picked the S35 files, which are further cropped in, however I don't understand why the "normal" M&S files use a completely different encoding (demosaiced YCbCr)?

@da-phil
Copy link
Contributor Author

da-phil commented Jun 5, 2023

I tried to look in this general direction, and the main stopping point for me is the said boolean interleaveRows.
What i have failed to understand from ITU-T81, is how we are supposed to derive the actual MCU size,
from the LJpeg itself?

@LebedevRI
When I rename the compressed ARW files to CR2 or DNG, rawspeed seems to still be able to decode the files and somehow deal with the "interleaving". I haven't understood all of the code to explain that to myself yet 😅
But this already shows the potential for refactoring & generalization of all this LJPEG stuff...

@kmilos
Copy link
Contributor

kmilos commented Jun 5, 2023

it seems I picked the S35 files, which are further cropped in,

Yes, the S35/APS-C crop is the same size in pixels as the M output in FF, but the M are demosaiced and full field of view (i.e. scaled down), while S35/APS-C are still CFAs, only physically cropped FOV.

why would you do that?

Even smaller files, if you don't care about the resolution, but maybe still care about dynamic range and want to push color and tone processing in post.

Again, different can of worms (CbCr sub-sampling and ITU-T81 interleaving might play a factor here), so I'd ignore M&S for now and tackle them separately later. The S35/APS-C cropped ARWs should just work once this first hurdle is done.

@kmilos
Copy link
Contributor

kmilos commented Jun 5, 2023

@LebedevRI Actually, after looking at the SOF3 and SOS headers again, there is component interleaving on the MCU level:

Sony SOF3 (14-bit, 256x256 frame, 4 components, 1x1 sampling each): FF C3 00 14 0E 01 00 01 00 04 01 11 00 02 11 00 03 11 00 04 11 00
Sony SOS (4 components interleaved, single entropy table, horiz. predictor): FF DA 00 0E 04 01 00 02 00 03 00 04 00 01 00 00

and, for Adobe DNG:

Adobe SOF3 (16-bit, 128x256 frame, 2 components, 1x1 sampling each): FF C3 00 0E 10 01 00 00 80 02 00 11 00 01 11 00
Adobe SOS (2 components interleaved, entropy table each, horiz. predictor): FF DA 00 0A 02 00 00 01 10 01 00 00

But the connection between the SOF3 4/2-component frame and the TIFF single channel CFA tile is still independent of ITU-T81 and on a different level of abstraction you (i.e. Adobe, Blackmagic, Sony) have to establish and maintain.

And similar for the Blackmagic case, where the number of components is 1, so there is even no MCU interleaving...

Blackmagic SOF3 (12-bit, 2064x1088, 1 component, 1x1 sampling): FF C3 00 0B 0C 04 40 08 10 01 00 11 00
Blackmagic SOS (1 component, predictor 6): FF DA 00 08 01 00 00 06 00 00

All of these are valid ITU-T81 - it doesn't know or care where the components it was fed came from in these 3 cases, but the bespoke TIFF decoder needs to know how to rearrange these SOF3 frames back into 512x512, 256x256, and 1032x2176 TIFF tiles respectively.

Recap: due to crop size roundup to multiples of tile size (512)
Sony lossless files exhibit black areas if not further cropped down
to actual pixel area, which is covered by the sensor.

Exif.SubImage1.0x7038 seems to reliably provide the true crop size,
whereas Exif.SubImage1.DefaultCropSize only crops to OOC jpeg size
losing some pixel area
@@ -145,6 +147,13 @@ RawImage ArwDecoder::decodeRawInternal() {
return mRaw;
}

if (7 == compression) {
DecodeLJpeg(raw);
// cropping of lossless compressed L files already done in Ljpeg decoder
Copy link
Contributor

@kmilos kmilos Jun 7, 2023

Choose a reason for hiding this comment

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

Not only L, but also APS-C/S35...

Could also leave a note that M/S are not CFA, and support is TBD. We also probably want to detect this somehow, if not throwing an exception earlier already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question is, where in the whole pipeline does the encoding fail, it is not even executing the ARW decoder in case of M/S images.

src/librawspeed/decoders/ArwDecoder.cpp Show resolved Hide resolved
@LebedevRI
Copy link
Member

Let me just take this over...

@LebedevRI
Copy link
Member

(Taking over in #483)

@LebedevRI LebedevRI closed this Jun 7, 2023
@da-phil
Copy link
Contributor Author

da-phil commented Jun 7, 2023

Let me just take this over...

would be great if you have some time to continue this work properly, but please let us know if you need help with some tasks, maybe with the easier ones ;)

@LebedevRI
Copy link
Member

c38746d

@LebedevRI
Copy link
Member

Let me just take this over...

would be great if you have some time to continue this work properly

Nah.

but please let us know if you need help with some tasks, maybe with the easier ones ;)

Yeah i wish i knew. Some small self-contained things could include:

  • Get rid of all hints.has() calls, there should be a way to tell from the raw itself, most of the time
  • Some cameras store black/white levels in EXIF, while some decoders already handle that, many do not (e.g. Canon)
  • Black area sampling is just completely broken
  • Black level handling needs to be refactored to support arbitrary number (and layout) of bins
  • ???

On a high level, i'm still stuck on the general idea to the likes of #325.

@LebedevRI
Copy link
Member

@da-phil thank you!

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.

3 participants