extmod/modframebuf: added GS4_HMSB format #2767

Open
wants to merge 2 commits into
from

Projects

None yet

3 participants

@kamikaze
Contributor
kamikaze commented Jan 7, 2017

Added 4bpp grayscale format. Tested on SSD1322 display

extmod/modframebuf.c
@@ -115,7 +171,7 @@ static inline uint32_t getpixel(const mp_obj_framebuf_t *fb, int x, int y) {
}
STATIC void fill_rect(const mp_obj_framebuf_t *fb, int x, int y, int w, int h, uint32_t col) {
- if (x + w <= 0 || y + h <= 0 || y >= fb->height || x >= fb->width) {
@kamikaze
kamikaze Jan 7, 2017 Contributor

I have a question about this part. What was a purpose of the part that I have changed? As for me it was pointless check but I could be wrong. So let's discuss this

@pfalcon
pfalcon Jan 7, 2017 Member

What was a purpose of the part that I have changed?

That's not a good question to ask ;-). (Akin to standing in a factory asking: what does that red button which I just pressed does?) Here it's clear that it checks whether a rectangle to be drawn and framebuffer's bounding rectangle intersect. If they don't, there's nothing to be drawn. If they do, there's something to draw, though perhaps a clipped part of the rectangle requested.

What's important is that this line has no relation to "adding GRAY4 format" whatsoever, so it shouldn't be a part of this change. And of course, before submitting a separate pull request to change it, you would first need to understand what it does (or otherwise just leave it alone). Take an example of #2751 - it's sad that @dpgeorge decided not to merge it, but it was done right - you addressed one issue in it, and it didn't contain unrelated changes. Please keep up doing it like that, not mix in different kind of changes like here.

@kamikaze
kamikaze Jan 7, 2017 Contributor

asking is always a good thing ) and actually fill_rect is a related part to gray4 fill rect ) I will return previous check. But as I see it - there is a need to check for zero height/width too. No need to call subroutines if there is nothing to do

@kamikaze
kamikaze Jan 7, 2017 Contributor

Extracted into separate PR #2771

@kamikaze kamikaze changed the title from extmod/modframebuf.c: added GRAY4 format to extmod/modframebuf: added GRAY4 format Jan 7, 2017
@dpgeorge

This is really an addition of a generic 4 bit-per-pixel format, it's not tied to gray pixels only. But considering we call the 16bpp format RGB565, I guess GRAY4 is an acceptable name.

But then also consider that there are 4 ways to have 4bpp (like with 1bpp): vert-msb, vert-lsb, horiz-msb, horiz-lsb. I think you have horiz-msb here, so it should be named something like GRAY4_HMSB.

Also, please provide (in a separate commit but same PR) tests :) You only need to provide unit tests for the new code you wrote: get pixel, set pixel, rect with various combinations of odd/even width and odd/even starting positions.

extmod/modframebuf.c
+ if (x % 2) {
+ *pixel = ((uint8_t)col & 0x0F) | (*pixel & 0xF0);
+ }
+ else {
@dpgeorge
dpgeorge Jan 9, 2017 Contributor

Please note style: all curly brackets on the one line.

extmod/modframebuf.c
+ uint8_t *pixel_pair = &((uint8_t*)fb->buf)[(x + y * fb->stride) >> 1];
+ uint8_t col_cleaned = (uint8_t)col & 0x0F;
+ uint8_t col_shifted_left = col_cleaned << 4;
+ uint8_t colored_pixel_pair = col_shifted_left | col_cleaned;
@dpgeorge
dpgeorge Jan 9, 2017 Contributor

I don't think you gain much by having these explicit variables. Can you check if the code size reduces (or stays the same) if you just repeat these in the code below? You can just do col &= 0x0f here and then use col << 4 below.

@kamikaze
kamikaze Jan 9, 2017 Contributor

First there was a for-loop instead of memset() and it had larger impact on speed. But it's still a good idea to calculate values (that will not change inside loops) outside. No unneeded memory allocations, no unneeded cpu usage aswell. Drawing graphics is not a strong part especially in case of I2C/SPI + bigger screen resolution. So I think it's better to do drawing as fast as possible everywhere it is possible. If we don't want a slideshow instead of animation.

And again - readability )

the only thing to optimize by removing is col_cleaned indeed.

@kamikaze
Contributor
kamikaze commented Jan 9, 2017

about GRAY4. I just don't know how to name it. Actually in datasheets it is called "grayscale" or "GS". So I could call it as "GS4_HMSB" or "GRAY4_HMSB"

@kamikaze
Contributor
kamikaze commented Jan 12, 2017 edited

renamed GRAY4 to GS4_HMSB (this name actually influences the fw size, +4 bytes)

@kamikaze kamikaze changed the title from extmod/modframebuf: added GRAY4 format to extmod/modframebuf: added GS4_HMSB format Jan 12, 2017
@kamikaze
Contributor
kamikaze commented Jan 12, 2017 edited

believe me or not, but removing pre-calculated variables INCREASED the fw size ))

before (GRAY4 has been renamed to GS4_HMSB at this point):

   text    data     bss     dec     hex filename
 302876     352   28240  331468   50ecc build-PYBV11/firmware.elf

after:

   text    data     bss     dec     hex filename
 302888     352   28240  331480   50ed8 build-PYBV11/firmware.elf
@kamikaze
Contributor

Added tests

@kamikaze
Contributor

What else can I do to get this accepted? )

kamikaze added some commits Jan 1, 2017
@kamikaze kamikaze extmod/modframebuf: added GS4_HMSB format 60c62de
@kamikaze kamikaze tests/extmod/framebuf4: added tests for GS4_HMSB format
72c17da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment