Skip to content
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

Raw10 unpacking is incorrect #3

Open
6by9 opened this issue Jan 22, 2018 · 3 comments
Open

Raw10 unpacking is incorrect #3

6by9 opened this issue Jan 22, 2018 · 3 comments

Comments

@6by9
Copy link

6by9 commented Jan 22, 2018

I realise that you've probably dropped this project, but it's just been noticed by a colleague that your unpacking of the raw10 is wrong. Pixel 0 is in the lowest 2 bits of the 5th byte, with pixel 3 in the highest 2 bits, as described in the CSI2 spec or https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-srggb10p.html. The code at https://github.com/illes/raspiraw/blob/master/raspi_dng.c#L172 has them in reverse order.
The same is true in @bablokb and @dword1511 forks of this tool.

@jason-curtis
Copy link

jason-curtis commented Dec 4, 2018

thanks for pointing this out @6by9 !

I have an interest in this since my team is now maintaining a Python alternative to this at https://github.com/OsmoSystems/picamraw/ . We'd certainly like our implementation to be correct 😄
I see how the implementation is backwards compared to the spec you provided (bits 0 and 1 high would be 0b00000011 not 0b11000000) .

It does seem plausible that this bug has gone uncaught through all implementations because in reality the little bits are often dominated by noise.

What makes you think the pi camera format conforms to the spec you provided?

@jason-curtis
Copy link

OK, I at least partially answered my own question, through forum spelunking.

The raspberry pi driver for the pi camera mentions this format at https://github.com/raspberrypi/linux/blob/1e72a58ef45d0a1bb53441166e98491a40a52072/drivers/media/platform/bcm2835/bcm2835-unicam.c#L251 . https://github.com/raspberrypi/linux/blob/1e72a58ef45d0a1bb53441166e98491a40a52072/include/uapi/linux/videodev2.h also seems built around this format.

Also I finally recognized your handle... If you're the Raspbery Pi engineer who wrote the Pi side implementations, I think maybe I'll just take you at your word 😆

@6by9
Copy link
Author

6by9 commented Dec 6, 2018

Yes, I work at Raspberry Pi.

Docs for V4L2 I had already linked to - https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-srggb10p.html

It's part of the CSI2 spec (I would link to it, but it's closed by the MIPI Alliance), and any CSI2 sensor will comply with it.
The CCP2 spec, predecessor to CSI2, can be found online (not sure if legitimately or not), and part 2 section 7.9 references exactly the same format.
P1[9:2] | P2[9:2] | P3[9:2] | P4[9:2] | P4[1:0] P3[1:0] P2[1:0] P1[1:0]

jason-curtis added a commit to OsmoSystems/picamraw that referenced this issue Dec 15, 2018
Bug reported by @6by9 at illes/raspiraw#3

## Notes:
- fixtures had to be updated
- for the 5th bytes, I switched to using `pixel_bytes_2d` as source data because the left-shift applied to `data` was breaking things. This has the consequence that we will now blow up if you pass a float array instead of an integer array (seems appropriate to me) but I had to change a test as a result

Testing done:
- Test-First Development: updated the appropriate unit test first before updating the code
- updated test fixtures with `np.save()` after changing the code

example failing fixture test before fixture update: noted that none of the printed values (I didn't do an exhaustive check) changed by more than the expected 4 DN
```python
    def test_integration(self):
        bayer_array = np.load(picamv2_BGGR_bayer_array_path)
        actual = module.bayer_array_to_rgb(bayer_array, BayerOrder.BGGR)

        expected = np.load(picamv2_rgb_path)

>       np.testing.assert_array_equal(actual, expected)
E       AssertionError:
E       Arrays are not equal
E
E       (mismatch 77.43123746172526%)
E        x: array([66. , 74.5, 91. , ..., 65. , 65. , 67. ])
E        y: array([64. , 74.5, 88. , ..., 65. , 66. , 66. ])
```
dword1511 added a commit to dword1511/raspiraw that referenced this issue Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants