fix: normalize 16-bit PNG images to 8-bit for PDF embedding#598
Open
Krishnanand-G wants to merge 1 commit into
Open
fix: normalize 16-bit PNG images to 8-bit for PDF embedding#598Krishnanand-G wants to merge 1 commit into
Krishnanand-G wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Changes16-bit PNG normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/gofpdf/parseimage_test.go`:
- Around line 38-59: The PNG normalization test only exercises the
*image.NRGBA64 path in TestFromBytes_Normalizes16BitPNG, so it misses the other
16-bit decode branches handled by is16BitImage. Extend this test to cover
*image.RGBA64 and *image.Gray16 as additional inputs through gofpdf.FromBytes,
and verify each is normalized to an 8-bit PNG after decoding img.Bytes. Keep the
checks anchored around TestFromBytes_Normalizes16BitPNG, mustDecodePNG, and
gofpdf.FromBytes so the regression coverage stays tied to the existing
normalization logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 013700f1-0af7-49e5-8b35-7193440eefd3
📒 Files selected for processing (3)
internal/providers/gofpdf/parseimage.gointernal/providers/gofpdf/parseimage_test.gointernal/providers/gofpdf/pngutil.go
Author
|
Addressed review feedback in 3daf134:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/gofpdf/parseimage_test.go`:
- Around line 82-87: The test in parseimage_test.go only checks the decoded PNG
type, so it can pass even when normalization produces the wrong 8-bit image.
Update the assertions around png.Decode and the switch cases in the
parseImage-related test to verify at least one known pixel value (and optionally
image bounds) for each expected conversion path, using the existing
parseImage/normalize output and decoded image symbols to validate the actual
pixel data, not just the concrete type.
In `@internal/providers/gofpdf/parseimage.go`:
- Around line 22-25: The PNG handling in FromBytes currently always routes
through normalizePNGBytes, which forces an unnecessary full png.Decode on every
PNG input. Update the PNG path in parseimage.go to inspect the PNG IHDR bit
depth first and only call normalizePNGBytes when the image is actually 16-bit;
keep the existing fast path unchanged for already 8-bit PNGs. Use the FromBytes
and normalizePNGBytes symbols to locate the conditional so the common case
avoids extra decoding and allocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a8cc577-4f58-48b2-99a4-282917508fa3
📒 Files selected for processing (2)
internal/providers/gofpdf/parseimage.gointernal/providers/gofpdf/parseimage_test.go
Krishnanand-G
force-pushed
the
fix/png-16bit-to-8bit-528
branch
3 times, most recently
from
a0994f5 to
c4ae85e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/providers/gofpdf/parseimage.go (1)
21-24: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winKeep the 8-bit PNG path on the fast path.
This still decodes every PNG before checking whether the payload is actually 16-bit. Already-8-bit PNGs now pay an extra full decode/allocation even though the helper returns the original bytes unchanged. Sniff the IHDR bit depth first and skip normalization unless it is
16.Suggested change
func normalizePNGBytes(data []byte) ([]byte, error) { + if len(data) < 26 || + !bytes.Equal(data[:8], []byte{137, 80, 78, 71, 13, 10, 26, 10}) || + !bytes.Equal(data[12:16], []byte("IHDR")) || + data[24] != 16 { + return data, nil + } + src, err := png.Decode(bytes.NewReader(data)) if err != nil { return data, nil }Also applies to: 36-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/gofpdf/parseimage.go` around lines 21 - 24, The PNG handling in parseImage is doing unnecessary work for already-8-bit images by always calling normalizePNGBytes before knowing the bit depth. Update parseImage to inspect the PNG IHDR bit depth first and only invoke normalizePNGBytes for 16-bit payloads, keeping the 8-bit PNG path on the fast path. Use the existing parseImage and normalizePNGBytes flow to preserve behavior while avoiding the extra decode/allocation for non-16-bit PNGs.internal/providers/gofpdf/parseimage_test.go (1)
82-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert a known pixel after normalization.
This only proves the decoded image is no longer one of the 16-bit concrete types. A broken conversion that returns the wrong 8-bit pixels would still pass. Compare at least one pixel (and ideally bounds) against the expected 8-bit value for each case.
Suggested change
cases := []struct { name string build func() image.Image + want color.Color }{ { name: "NRGBA64", build: func() image.Image { m := image.NewNRGBA64(image.Rect(0, 0, 2, 2)) m.SetNRGBA64(0, 0, color.NRGBA64{R: 65535, G: 32768, B: 16384, A: 65535}) return m }, + want: color.NRGBA{R: 0xFF, G: 0x80, B: 0x40, A: 0xFF}, }, { name: "RGBA64", build: func() image.Image { m := image.NewRGBA64(image.Rect(0, 0, 2, 2)) m.SetRGBA64(0, 0, color.RGBA64{R: 65535, G: 32768, B: 16384, A: 65535}) return m }, + want: color.NRGBA{R: 0xFF, G: 0x80, B: 0x40, A: 0xFF}, }, { name: "Gray16", build: func() image.Image { m := image.NewGray16(image.Rect(0, 0, 2, 2)) m.SetGray16(0, 0, color.Gray16{Y: 32768}) return m }, + want: color.Gray{Y: 0x80}, }, } @@ - var input bytes.Buffer - require.NoError(t, png.Encode(&input, tc.build())) + src := tc.build() + var input bytes.Buffer + require.NoError(t, png.Encode(&input, src)) @@ decoded, err := png.Decode(bytes.NewReader(img.Bytes)) require.NoError(t, err) + assert.Equal(t, src.Bounds(), decoded.Bounds()) switch decoded.(type) { case *image.NRGBA64, *image.RGBA64, *image.Gray16: t.Fatalf("expected 8-bit PNG, got %T", decoded) } + wr, wg, wb, wa := tc.want.RGBA() + r, g, b, a := decoded.At(0, 0).RGBA() + assert.Equal(t, wr, r) + assert.Equal(t, wg, g) + assert.Equal(t, wb, b) + assert.Equal(t, wa, a) }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/gofpdf/parseimage_test.go` around lines 82 - 87, The PNG normalization test in parseimage_test.go only checks that decode does not return 16-bit concrete types, so it can miss incorrect pixel conversion. Update the test around png.Decode and the type switch to also assert the normalized image bounds and compare at least one known pixel value against the expected 8-bit result for each case, ensuring ParseImage returns correctly converted 8-bit pixels rather than just the right image type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/providers/gofpdf/parseimage_test.go`:
- Around line 82-87: The PNG normalization test in parseimage_test.go only
checks that decode does not return 16-bit concrete types, so it can miss
incorrect pixel conversion. Update the test around png.Decode and the type
switch to also assert the normalized image bounds and compare at least one known
pixel value against the expected 8-bit result for each case, ensuring ParseImage
returns correctly converted 8-bit pixels rather than just the right image type.
In `@internal/providers/gofpdf/parseimage.go`:
- Around line 21-24: The PNG handling in parseImage is doing unnecessary work
for already-8-bit images by always calling normalizePNGBytes before knowing the
bit depth. Update parseImage to inspect the PNG IHDR bit depth first and only
invoke normalizePNGBytes for 16-bit payloads, keeping the 8-bit PNG path on the
fast path. Use the existing parseImage and normalizePNGBytes flow to preserve
behavior while avoiding the extra decode/allocation for non-16-bit PNGs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b371b96c-136b-45ec-901a-92c031c9110a
📒 Files selected for processing (2)
internal/providers/gofpdf/parseimage.gointernal/providers/gofpdf/parseimage_test.go
Krishnanand-G
force-pushed
the
fix/png-16bit-to-8bit-528
branch
from
c4ae85e to
44af67f
Compare
Author
|
Addressed CodeRabbit feedback in 44af67f:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Closes #528.
16-bit per channel PNGs fail when registering with gofpdf. This converts 16-bit PNG payloads to 8-bit NRGBA before embedding.
Changes
NRGBA64,RGBA64, andGray16PNG payloads inFromBytesTest plan
go test ./internal/providers/gofpdf/... -run TestFromBytes -v