Skip to content

Commit

Permalink
LJpegDecoder: Maximal output tile size should be a multiple of LJpeg …
Browse files Browse the repository at this point in the history 
…frame size
  • Loading branch information
@LebedevRI
LebedevRI committed Mar 25, 2024
1 parent 3ee4910 commit 63f7170
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions src/librawspeed/decompressors/LJpegDecoder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ Buffer::size_type LJpegDecoder::decodeScan() {
if (maxRes.area() != N_COMP * jpegFrame.dim.area())
ThrowRDE("LJpeg frame area does not match maximal tile area");

if (maxRes.x % jpegFrame.dim.x != 0 || maxRes.y % jpegFrame.dim.y != 0)
ThrowRDE("Maximal output tile size is not a multiple of LJpeg frame size");

int numRowsPerRestartInterval;
if (numMCUsPerRestartInterval == 0) {
// Restart interval not enabled, so all of the rows
Expand Down

9 comments on commit 63f7170

@kmilos
Copy link
Collaborator

@kmilos kmilos commented on 63f7170 Mar 26, 2024

Choose a reason for hiding this comment

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

Ah, this new test might still fail for the Blackmagic/CinemaDNG case?

@LebedevRI
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know yet.

@LebedevRI
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, out of 7 raws on RPU that have predictor != 1, this check does not fail for just 1 (one) of them.

@kmilos
Copy link
Collaborator

@kmilos kmilos commented on 63f7170 Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, out of 7 raws on RPU that have predictor != 1, this check does not fail for just 1 (one) of them.

Expected - this is I guess the Apple ProRaw 3 component predictor 7 case where there is no tile reshaping. I guess this is the first/easiest one to try to add support for.

For the BlackMagic/CinemaDNG case, jpegFrameDim.x = 2 * maxRes.x, not the other way around.

Perhaps the area check is sufficient?

@LebedevRI
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, out of 7 raws on RPU that have predictor != 1, this check does not fail for just 1 (one) of them.

Expected - this is I guess the Apple ProRaw 3 component predictor 7 case where there is no tile reshaping. I guess this is the first/easiest one to try to add support for.

Yup, that's the idea, and is exactly what i hoped would happen - small piecemeal separately observable steps.

For the BlackMagic/CinemaDNG case, jpegFrameDim.x = 2 * maxRes.x, not the other way around.

Perhaps the area check is sufficient?

Unless we literally need to drop a half of every row, the area check is clearly not sufficient,
because we'll still need to detect that other "layout". There's really only two possible cases:

  • either that results in canon-like layout, which looks similar to how a print looks, when printing two pages per sheet
  • or, more likely IMO, each LJpeg row encodes two consecutive raw rows.

@kmilos
Copy link
Collaborator

@kmilos kmilos commented on 63f7170 Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, that's the idea, and is exactly what i hoped would happen - small piecemeal separately observable steps.

Great! Unfortunately the Apple files now bail at IFD overlap check, and they didn't before... Maybe related to 0d88638?

the area check is clearly not sufficient

I might be thick and just don't get it... Even the DNG spec says:

For lossless JPEG, the internal width/length/components in the JPEG stream are not required to match the strip or tile's width/length/components. Only the total sample counts need to match. It is common for CFA images to be encoded with a different width, length or component count to allow the JPEG compression predictors to work across like colors.

The way I see it (and the drawing of the known cases matches), the only thing that matters is tiff_tile_comp * tiff_tile_w * tiff_tile_h = jpeg_frame_comp * jpeg_frame_w * jpeg_frame_h, i.e. "total sample count" (and what I meant by the existing "area" check a few lines above).

@LebedevRI
Copy link
Member Author

Choose a reason for hiding this comment

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

the area check is clearly not sufficient

I might be thick and just don't get it... Even the DNG spec says:

For lossless JPEG, the internal width/length/components in the JPEG stream are not required to match the strip or tile's width/length/components. Only the total sample counts need to match. It is common for CFA images to be encoded with a different width, length or component count to allow the JPEG compression predictors to work across like colors.

The way I see it (and the drawing of the known cases matches), the only thing that matters is tiff_tile_comp * tiff_tile_w * tiff_tile_h = jpeg_frame_comp * jpeg_frame_w * jpeg_frame_h, i.e. "total sample count" (and what I meant by "area" check a few lines above).

Sure. The check isn't there to be petty/picky and discard these raws.
It's only there to somehow differentiate between these LJpeg "flavors",
it will be adjusted if needed to support that other "layout".

@kmilos
Copy link
Collaborator

@kmilos kmilos commented on 63f7170 Apr 5, 2024

Choose a reason for hiding this comment

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

Unfortunately the Apple files now bail at IFD overlap check

For some reason, RawSpeed now tries to parse the Apple MakerNote as a vanilla IFD...

@kmilos
Copy link
Collaborator

@kmilos kmilos commented on 63f7170 Apr 5, 2024

Choose a reason for hiding this comment

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

Unfortunately the Apple files now bail at IFD overlap check

For some reason, RawSpeed now tries to parse the Apple MakerNote as a vanilla IFD...

Never mind, seems this was always the case and the exception does not stop execution, so should be good to proceed w/ this case.

Please sign in to comment.