[LDM3D] Fix rgblike_to_depthmap truncating 16-bit depth back to 8 bits#14200
[LDM3D] Fix rgblike_to_depthmap truncating 16-bit depth back to 8 bits#14200chuenchen309 wants to merge 1 commit into
Conversation
VaeImageProcessorLDM3D.rgblike_to_depthmap combines two 8-bit channels into a 16-bit depth value (green * 256 + blue, range 0-65535). It correctly casts up to a wider integer type for the multiplication, but then cast the result back to the input's dtype (uint8) via `.to(original_dtype)` / `.astype(original_dtype)`, dropping the entire high byte -- so it returned only `depth % 256` (e.g. 1480 came back as 200). numpy_to_depth feeds this into `Image.fromarray(..., mode="I;16")`, which requires 16-bit-wide data, so with a uint8 buffer `VaeImageProcessorLDM3D.postprocess(image, output_type="pil")` -- the normal way an LDM3D pipeline returns a depth image for a 6-channel output -- crashed with `ValueError: buffer is not large enough`. Keep the 16-bit result instead of truncating: return the wider integer type directly (numpy -> uint16 to match the `mode="I;16"` consumer, torch -> the int32 combination). The function's whole purpose is to produce a 16-bit depth map from 8-bit RGB, so casting back to the 8-bit input dtype was never correct. This regression was introduced by huggingface#12546 (which fixed a real NumPy 2.0 `uint8 * 256` overflow but replaced it with this truncation). The existing tests/others/test_image_processor.py had no VaeImageProcessorLDM3D coverage, which is why it shipped; this adds a small regression test class covering the 16-bit combination (numpy + torch) and the pil postprocess path. Verified against the real code before/after: rgblike_to_depthmap(green=5, blue=200) returned 200 (uint8) before and 1480 (uint16 / int32) after; postprocess(output_type="pil") raised ValueError before and returns a valid "I;16" depth image after. Full tests/others/test_image_processor.py: 12 passed. ruff 0.9.10 (repo-pinned) check + format: clean. Note (separate, not addressed here to keep this focused): the output_type="np" branch of postprocess passes float [0, 1] arrays into rgblike_to_depthmap, where the int cast collapses them to 0; that is an independent caller-side issue in the same function's contract and is better handled in its own change.
|
Hi @chuenchen309, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. |
|
Belated, but owed: I used an AI agent on this PR and didn't run your Self-review report — rubric: Blocking issuesNone found. Non-blocking issues
Dead code (advisory)
No new model/pipeline here, so the new-model trace doesn't apply. Suggestions / additional info
Left for the actual review, deliberately not fixed
VerdictREADY, with the comment trim above as the one thing I'd change if you want it. No blocking issues; the behavioural claim (16-bit result reaches the |
|
Following up on my own self-review note above: I went and read the Coding with AI agents guide properly, which the template links and I hadn't opened. It asks for three things in the PR description that I didn't provide:
On (1): I opened the PR without an issue at all. Filed #14206 now so the discussion has somewhere to live, but I'm aware that's backwards — the point of coordinating first is that you get to say "no" or "do it differently" before anyone reviews code. Happy to close this PR and wait on #14206 instead if you'd rather reset the order properly; no argument from me. On (2), the commands and output: The three added cases are red on |
What does this PR do?
VaeImageProcessorLDM3D.rgblike_to_depthmapcombines two 8-bit channels into a 16-bit depth value (green * 256 + blue, range 0–65535). It correctly casts up to a wider integer type for the multiplication, but then casts the result back to the input's dtype (uint8) via.to(original_dtype)/.astype(original_dtype), dropping the entire high byte — so it returned onlydepth % 256.numpy_to_depthfeeds this intoImage.fromarray(..., mode="I;16"), which requires 16-bit-wide data, so with a uint8 bufferVaeImageProcessorLDM3D.postprocess(image, output_type="pil")— the normal way an LDM3D pipeline returns a depth image for a 6-channel output — crashes withValueError: buffer is not large enough.The fix keeps the 16-bit result instead of truncating: return the wider integer type directly (numpy →
uint16to match themode="I;16"consumer, torch → theint32combination). The function's whole purpose is to produce a 16-bit depth map from 8-bit RGB, so casting back to the 8-bit input dtype was never correct.This regression was introduced by #12546 (which fixed a real NumPy 2.0
uint8 * 256overflow but replaced it with this truncation).tests/others/test_image_processor.pyhad noVaeImageProcessorLDM3Dcoverage, which is why it shipped — this adds a small regression test class.Before submitting
Ldm3dImageProcessorTest: 16-bit combination for numpy + torch, and the pil postprocess path; red before the fix, green after. Fulltest_image_processor.py: 12 passed.ruff0.9.10 check + format clean.)Who can review?
@yiyixuxu @sayakpaul
Found via an AI-assisted (Claude) review pass over
image_processor.py; independently reproduced, fixed, and verified before submitting.