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

JPEGDEC: Sync with latest upstream #948

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

Gadgetoid
Copy link
Member

Mostly in response to #941

@bitbank2 there are a few commits here that might be appropriate upstream. Notably:

  1. JPEG_setFramebuffer is forward-declared static, but not defined as static
  2. ucColMask is defined but never used
  3. iTable throws a sign compare- I looked over the code and figured it could be a uint16_t rather than an int to fix this, but am not sure if I'm missing some case where it might need to go negative or VERY BIG.

@Gadgetoid
Copy link
Member Author

Welp I just wasted my time trying to add support for a feature in a rejected pull request... 🙄

I guess this sync is probably still useful but I'm sticking it on hold for now. It would be more useful to update our bindings to better handle load/decode errors.

@bitbank2
Copy link

bitbank2 commented Jun 4, 2024

Welp I just wasted my time trying to add support for a feature in a rejected pull request... 🙄

I guess this sync is probably still useful but I'm sticking it on hold for now. It would be more useful to update our bindings to better handle load/decode errors.

who rejected what pull request? I was going to take a look at this today.

@Gadgetoid
Copy link
Member Author

who rejected what pull request? I was going to take a look at this today.

The linked issue linked to what I assumed was a merged commit to add cropping support. I failed to see the "Closed" at the top. 🫠

@bitbank2
Copy link

bitbank2 commented Jun 4, 2024

I added JPEG cropping features and some other cleanup to my own copy, but hesitated to share it. I've been less willing to "give it all away" lately. The crop feature is done and waiting for me to share it, but I'm not sure how to move forward from here. I had a loss of faith in open source recently.

@Gadgetoid
Copy link
Member Author

I had a loss of faith in open source recently.

Ooof. Dare I ask what happened?

To be clear, I wasn't asking for this specific feature, but since someone came along and said "hey it exists" I figured I'd support it. It just transpired that it didn't exist 😆

If you feel you need to hold back some shinies from the public domain for whatever reason- I totally understand!

@Gadgetoid
Copy link
Member Author

Big ooof-

CRITICAL ERROR: Block device / binary overlap!
Binary ends at 0x100a0560, block device starts at 0x100a0000
Processing: build-pico/pico-6cbf0fa0654672e92c30b670069e87383523d95c-pimoroni-micropython.uf2

@Gadgetoid
Copy link
Member Author

Looks like the change from:

//
// Clip and convert red value into 5-bits for RGB565
//
static const uint16_t usRangeTableR[] = {0x0000, // 0
0x0800,
0x1000,
0x1800,
0x2000,
0x2800,
0x3000,
0x3800,
0x4000,
0x4800,
0x5000,
0x5800,
0x6000,
0x6800,
0x7000,
0x7800,
0x8000,
0x8800,
0x9000,
0x9800,
0xa000,
0xa800,
0xb000,
0xb800,
0xc000,
0xc800,
0xd000,
0xd800,
0xe000,
0xe800,
0xf000,
0xf800,
0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800, // 32
0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,
0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,
0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,0xf800,
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, // 64
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, // 96
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};

to:

https://github.com/bitbank2/JPEGDEC/blob/4c864911ba49da30893df7e61c92834fa37f11c2/src/jpeg.inl#L256-L354

And similar, eats up more flash than we can spare. Some builds are really on the edge, so it's not surprising.

It looks like it should be perfectly possible to revert back to the older range tables, at the cost of output image quality and probably loss of SIMD support.

The difference is just a bit-shift:

usPixel = usRangeTableB[((iCBB + iY) >> 15) & 0x7f]; // blue pixel
usPixel |= usRangeTableG[((iCBG + iCRG + iY) >> 14) & 0xff]; // green pixel
usPixel |= usRangeTableR[((iCRR + iY) >> 15) & 0x7f]; // red pixel

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

Successfully merging this pull request may close these issues.

2 participants