-
Notifications
You must be signed in to change notification settings - Fork 115
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
Closed
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b383e9e
ARW: Support new LJPEG compression on ILCE-7M4 and ILCE-7R5
da-phil 25585e2
Use Exif.SubImage1.0x7038 field for Sony lossless compression crop
da-phil 1972397
Boyscout: make sony exif tag naming consistent
da-phil 5f3fe8f
Do not apply crop from cameras.xml and set crop origin to 0,0
da-phil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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 beforedecodeMetaData()
(both calling their inherited decoder specific function), so this should work. Will give it a try now...There was a problem hiding this comment.
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 bottomIf 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...There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?