[bug] fix llava processor to calculate unpadding size correctly
Author: cyr0930
Pull request: yes

# What does this PR do?
Additional fix for https://github.com/huggingface/transformers/pull/37819.
I didn't fix _get_unpadded_features methods in llava family processor.

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#create-a-pull-request),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [x] Did you write any new necessary tests?


## Who can review?
@zucchini-nlp



Comments:
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the `Ready for review` button (at the bottom of the PR page). This will assign reviewers and trigger CI.

Sorry I mistakenly didn't fix the _get_unpadded_features metheds in prev PR

@zucchini-nlp  is this the reason the TRL CI tests are failing for odd-sized images for `LlavaNextForConditionalGeneration`? See here: https://github.com/huggingface/trl/actions/runs/14881264112/job/41789730121

@kashif yes, seems like it. We'll add a test on our side as well with odd image sizes 

we had:
```python
     dataset_dict = {
            "prompt": [
                [{"role": "user", "content": [{"type": "image"}, {"type": "text", "text": "Describe the image in great detail."}]}],
                [{"role": "user", "content": [{"type": "image"}, {"type": "text", "text": "Is this bus in the USA?"}]}],
                [{"role": "user", "content": [{"type": "image"}, {"type": "text", "text": "Give a thorough description of the image."}]}],
                [{"role": "user", "content": [{"type": "image"}, {"type": "text", "text": "Who are the people in the image?"}]}],
                [{"role": "user", "content": [{"type": "image"}, {"type": "text", "text": "What is written?"}]}],
            ],
            "chosen": [
                [{"role": "assistant", "content": [{"type": "text", "text": "The image features a modern, multi-colored train."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": "Yes, it can be assumed that this bus is in the USA."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": "The image features a forest path."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": "There are two individuals, possibly girls or women."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": '"ccpb".'}]}],
            ],
            "rejected": [
                [{"role": "assistant", "content": [{"type": "text", "text": "The image features a modern, colorful train."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": "No, it's not in the USA."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": "The image features a forest path surrounded by trees."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": "In the image, there are two individuals."}]}],
                [{"role": "assistant", "content": [{"type": "text", "text": '"ccpb".'}]}],
            ],
            "images": [
                [Image.fromarray(np.random.randint(0, 255, (92, 33, 3), dtype=np.uint8))],
                [Image.fromarray(np.random.randint(0, 255, (64, 48, 3), dtype=np.uint8))],
                [Image.fromarray(np.random.randint(0, 255, (80, 152, 3), dtype=np.uint8))],
                [Image.fromarray(np.random.randint(0, 255, (57, 24, 3), dtype=np.uint8))],
                [Image.fromarray(np.random.randint(0, 255, (102, 48, 3), dtype=np.uint8))],
            ],
        }
```

> Right, thanks for noticing it and fixinff! Left a few comments on new tests and we can add a couple slow tests with real checkpoints from the hub :)

Thanks for the review. I'll check and fix what is needed :)

The simplest edge case looks like the following image which should not be unpadded.
![image](https://github.com/user-attachments/assets/caf494db-ca40-472b-a6c2-7324835bb118)

In the previous logic (https://github.com/huggingface/transformers/blob/v4.51.3/src/transformers/models/llava_next/modeling_llava_next.py#L116), new_height is computed with round, so it result in unpadding both top and bottom, which is wrong.

And in the refined logic (https://github.com/huggingface/transformers/pull/37819/commits/cc581aba6e4b93e980c5bf13d86de2ef1da91693#diff-24741085a5a99c475d86aa2d98b106a6d3b8685375a2bcc4803f47bd5d2b8852R143), new_height is computed with math.ceil and padding could contain residual, so it results in unpadding bottom, which is also incorrect.

Another problem is these computation use patch-leveled length. If only one pixel occupies the bottom (or right)-most patch, that patch should not be classified as a padding patch. However, patch-leveled computation can't cover this, because it lost some precision.

In conclusion, unpadding logic should use recovered length IMO.

If this is valid and the commits look okay to you, I will fix them for llava_next_video and llava_onevision too.

@cyr0930 Thanks for detailed information! It makes sense for me that we can't unpad on `recovered length` since the features are already patchified by the time unpadding happens. I'd prefer to leave everything as is and adapt the logic for expanding placeholder tokens, because we're currently over-engineering and diverging from original implementation

Also, let's add an integration test with official weights, instead of the dummy test we have currently. Our main goal is to make sure the models can do forward pass correctly. We can even use the same image sizes provided above, which are failing for TRL 😉 

> @cyr0930 Thanks for detailed information! It makes sense for me that we can't unpad on `recovered length` since the features are already patchified by the time unpadding happens. I'd prefer to leave everything as is and adapt the logic for expanding placeholder tokens, because we're currently over-engineering and diverging from original implementation
> 
> Also, let's add an integration test with official weights, instead of the dummy test we have currently. Our main goal is to make sure the models can do forward pass correctly. We can even use the same image sizes provided above, which are failing for TRL 😉

okay I see :)
As it could be somewhat different from original inplementation, I'll rollback some of them.
Actually removing or adding one or two padded features doesn't make that much difference as vision encoder has larger receptive field, so it can cover edge case image in some sense.
Thanks for the review! I will also fix the test cases to integration test.

IMO reverting the unpad logic as it was is more relevant, because unpad logic doesn't have to be the same as pad logic. As we observed, unpad logic uses different unit (num_pad), and removing one pixel vs removing one padding feature is quite different.
In the same manner, test with unpad is no longer relevant, only checking # image tokens and # image features would be fine. Therefore, I removed unpad_test.
And I don't think more test is required as this part (https://github.com/huggingface/transformers/blob/v4.51.3/src/transformers/models/llava_next/modeling_llava_next.py#L643) always check if # image tokens == # image features, so all the existing integration tests can't be passed if unpad logic is wrong.

> Thanks! Last comment, let's not use a processor in model tests, we usually prepare dummy inputs and use them instead
> 
> I'll approve and you can ping me to merge when the comment is addressed

Thanks for the reviews!

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_37988). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.