Add fill method to displayio.Bitmap

[caternuson]

This is functional at least. I basically just stole from the pixel set code and loopified it.

Test program:

import time
import board
import displayio

WIDTH = board.DISPLAY.width
HEIGHT = board.DISPLAY.height

COLORS = (0xFF0000, 0x00FF00, 0x0000FF)

bitmap = displayio.Bitmap(WIDTH, HEIGHT, len(COLORS))

palette = displayio.Palette(len(COLORS))
for i, color in enumerate(COLORS):
    palette[i] = color

tile_grid = displayio.TileGrid(bitmap, pixel_shader=palette)

splash = displayio.Group()
splash.append(tile_grid)

board.DISPLAY.show(splash)

print("Fill 1")
board.DISPLAY.auto_refresh = False
start = time.monotonic()
for i in range(WIDTH*HEIGHT):
    bitmap[i] = 1
print(time.monotonic() - start)
board.DISPLAY.auto_refresh = True

time.sleep(1)

print("Fill 2")
start = time.monotonic()
bitmap.fill(2)
print(time.monotonic() - start)

Results:

Adafruit CircuitPython 5.1.0-91-g49fff2d9b-dirty on 2020-04-09; Adafruit CLUE nRF52840 Express with nRF52840
>>> import fill_test
Fill 1
1.32599
Fill 2
0.0269775
>>> 

[kevinjwalters]

Looks like uint32_t bytes_per_value = self->bits_per_value / 8; is constant and can be placed outside both for loops to avoid repetition?

You could also swap the loop order and do y as the oustide loop to reduce execution of int32_t row_start = y * self->stride;

[kevinjwalters]

Are .x2 and .y2 exclusive values for the dirty region? I.e. if a 4x4 bitmap is being cleared should they be set to 3 or 4? The current proposed code sets them to 4.

[caternuson]

For fill, I thought I did move bytes_per_value outside the loop?

Good question about .x2 and .y2. I based these on what is done in the constructor. The low ends are set to 0, so maybe these should be -1ed?

[kevinjwalters]

Apologies, I didn't check second diff as I thought it was just doc changes.

[caternuson]

No worries. First commit was "copy, paste, does it work?". Second commit had some minor refactoring.

[jepler]

Normally in Python lower-bounds are inclusive and upper-bounds are exclusive, so the slice [1:4] is the elements 1, 2, and 3. I hope that the same is chosen for x2/y2 in this PR.

[tannewt]

This approach is fine with me.

Do you want to speed it up further? You could pack the value into one uint32_t at the start and then fill the memory. That would reduce the number of memory operations.

[kevinjwalters]

@tannewt That's funny, I was just went for a stroll and thought of the same thing for the special but common easy case of 0.

I've not looked at exactly how the packing works, will one 32bit chunk work for all cases? For 8 palette colours represented by 3bits does it spill between two 32bit chunks, i.e. would need three different 32 bit values to be written in?

[caternuson]

@tannewt Sure, might as well make it as fast as possible while we're here. Guess I have the same general question as @kevinjwalters.

[tannewt]

bits_per_value is always a power of 2 (so 1, 2 or 4 bits). Inherent in the computation here: https://github.com/adafruit/circuitpython/blob/master/shared-bindings/displayio/Bitmap.c#L66

You may accidentally set values past the end of the row but that is fine because the stride (aka how long a row is) is rounded up to the nearest word boundary.

[caternuson]

Something like this? Tested this and seems to work.

    // build the packed word
    uint32_t word = 0;
    for (uint8_t i=0; i<32 / self->bits_per_value; i++) {
        word |= (value & self->bitmask) << (32 - ((i+1)*self->bits_per_value));
    }
    // copy it in
    for (uint32_t i=0; i<self->stride * self->height; i++) {
        self->data[i] = word;
    }

EDIT - oh, and is indeed faster :)

Adafruit CircuitPython 5.1.0-92-gdc7574684-dirty on 2020-04-10; Adafruit CLUE nRF52840 Express with nRF52840
>>> import fill_test
Fill 1
1.32199
Fill 2
0.0059967
>>> 

[tannewt]

@caternuson yup! Looks right to me.

[caternuson]

@tannewt OK, pushed those changes. I think the last remaining question is the treatment of the upper bounds for the dirty area.

This? (current code)

    self->dirty_area.x2 = self->width;
    self->dirty_area.y2 = self->height;

or this?

    self->dirty_area.x2 = self->width - 1;
    self->dirty_area.y2 = self->height - 1;

[kevinjwalters]

Looks like the original code was correct with x2 and y2 setting judging by comment describing x2 in https://github.com/adafruit/circuitpython/blob/master/shared-module/displayio/area.h#L33-L39 ...

[tannewt]

Current version looks correct. Thanks!