To displayio.Display: expose fill_area and add a property for rotation #2027

dastels · 2019-07-31T22:54:39Z

No description provided.

tannewt

Thank you for adding this! Just a couple comments on memory allocation.

tannewt · 2019-08-02T18:05:34Z

shared-bindings/displayio/Display.c

+
+    displayio_display_fill_area(self, &area, mask, buffer);
+
+    mp_obj_array_t *result = array_new(BYTEARRAY_TYPECODE, buffer_size * 4);


Instead of allocating an array, please accept a bytearray or array in as an argument so that the buffer can be reused. Reusing the buffer will make it easier to allocate and reduce fragmentation.

tannewt · 2019-08-02T18:06:31Z

shared-bindings/displayio/Display.c

+/*     .proxy = {(mp_obj_t)&displayio_display_get_screenshot_obj, */
+/*               (mp_obj_t)&mp_const_none_obj, */
+/*               (mp_obj_t)&mp_const_none_obj}, */
+/* }; */


Please remove this attribute.

shared-bindings/displayio/Display.c

dastels · 2019-08-07T19:29:11Z

OK, Scott. I addressed the issues above. You now pass in an adequately large bytearray. Exceptions raised if it's not a bytearray or if it's not big enough.

tannewt

I've looked at this more closely and am worried it won't work for any arbitrary area. I looked at your Python library on top and it's going row by row to output to a bitmap.

Let's embrace that specific scenario and reduce the api down to fill_row where it's given a y and an array of 16bit values that is width long. Also check that the display has a 16 bit colorspace and raise an exception otherwise.

The rows_per_buffer section can all be simplified away because we're only doing one row at a time then and we know it is aligned well. The given buffer can then be passed into fill_area directly instead of having to do a copy afterwards.

Thanks and let me know if you have any questions.

deshipu · 2019-08-09T22:40:58Z

shared-bindings/displayio/Display.c

+    mp_int_t y = args[ARG_y].u_int;
+    mp_int_t w = args[ARG_width].u_int;
+    mp_int_t h = args[ARG_height].u_int;
+    mp_obj_array_t *result = (mp_obj_array_t *)(args[ARG_buffer].u_obj);


I would recommend using mp_get_buffer_raise to get the buffer and its size, then you don't need all those functions for accessing the array elements, etc.

You can see an example at https://github.com/adafruit/circuitpython/pull/2032/files#diff-28511b511b1f39aef03beef1a21a603eR82-R87

Since this was the usecase, doing so simplifies the function significantly.

tannewt

Thanks for the simplification of the API! I added a couple of comments to further simplify the implementation too.

tannewt · 2019-08-20T03:39:37Z

shared-bindings/displayio/Display.c

+
+//|   .. method:: fill_row(y, buffer)
+//|
+//|     Extract the pixels fro a single row


Suggested change

//| Extract the pixels fro a single row

//| Extract the pixels from a single row

tannewt · 2019-08-20T03:41:44Z

shared-bindings/displayio/Display.c

+  enum { ARG_y, ARG_buffer };
+    static const mp_arg_t allowed_args[] = {
+        { MP_QSTR_y, MP_ARG_INT | MP_ARG_KW_ONLY, {.u_int = -1} },
+        { MP_QSTR_buffer, MP_ARG_OBJ | MP_ARG_KW_ONLY, {} },


I'd remove both MP_ARG_KW_ONLYs and make them required positional arguments.

tannewt · 2019-08-20T03:47:53Z

shared-bindings/displayio/Display.c

+
+    displayio_display_fill_area(self, &area, mask, buffer);
+
+    if ((result->len + result->free) >= (buffer_size * 4)) {


Please do this check before fill_area and then pass the given buffer straight into fill_area. That will simplify this implementation further.

tannewt

Looking better! Just one more re-arrangement.

tannewt · 2019-08-20T17:03:08Z

shared-bindings/displayio/Display.c

+    }
+
+    mp_buffer_info_t bufinfo;
+    mp_get_buffer_raise(result, &bufinfo, MP_BUFFER_WRITE);


Do this above where you have the array cast before the typecode check otherwise you are testing against memory that may not be what you think it is.

dastels · 2019-08-21T00:44:37Z

@tannewt

tannewt

Please get the first CI job green by fixing the test unix build. Once it's down to the itsy m0 build and space then I'll fix it for you.

tannewt · 2019-08-21T17:52:10Z

shared-bindings/displayio/Display.c

+    mp_buffer_info_t bufinfo;
+    mp_get_buffer_raise(result, &bufinfo, MP_BUFFER_WRITE);
+
+    if (result->typecode != BYTEARRAY_TYPECODE) {


Suggested change

if (result->typecode != BYTEARRAY_TYPECODE) {

if (bufinfo.typecode != BYTEARRAY_TYPECODE) {

dastels · 2019-08-22T02:01:25Z

OK, @tannewt , travis is green.

tannewt

Thank you for adding this and your patience with my feedback!

dastels added 5 commits July 25, 2019 15:04

Expose array_new and array_subscr

7fd1a6c

WIP on exposing fill_area

b6178c9

Get fill_area working

741cd9c

Expose rotation with a property

1f9cb44

Merge remote-tracking branch 'adafruit/master' into displayio_fill_area

cd092df

ladyada requested a review from tannewt August 1, 2019 17:51

tannewt added displayio enhancement labels Aug 2, 2019

tannewt added this to the 5.x milestone Aug 2, 2019

tannewt requested changes Aug 2, 2019

View reviewed changes

dastels added 2 commits August 7, 2019 15:27

Remove obsolete experimental property

263f6f4

Pass in preallocated result buffer

239ad19

tannewt requested changes Aug 9, 2019

View reviewed changes

deshipu reviewed Aug 9, 2019

View reviewed changes

Simplify to only extracting one line

7a235f3

Since this was the usecase, doing so simplifies the function significantly.

tannewt requested changes Aug 20, 2019

View reviewed changes

dastels added 4 commits August 20, 2019 10:03

Fix typo

2b7897c

Switch to positional parameters

10bc0d2

Rework to simplify

53bb95a

Remove temporarily comments code

f3d476a

tannewt requested changes Aug 20, 2019

View reviewed changes

dastels added 3 commits August 20, 2019 13:45

Remove array cast

0fd886f

Move the get_buffer call earlier

eb0a8cc

getting the buffer info should happen first (due to its check)

56aad05

tannewt requested changes Aug 21, 2019

View reviewed changes

dastels added 3 commits August 21, 2019 13:57

Remove unneeded lines

8bbab01

Make them static (fixing build break?) as they aren't needed by Display

5023b62

Add message ids for translation

8b2a1b9

dastels added 3 commits August 21, 2019 14:46

Make room

dcf36db

Merge remote-tracking branch 'adafruit/master' into displayio_fill_area

d547d59

Get rid of the last bits of array dependancy

99a3da3

tannewt approved these changes Aug 22, 2019

View reviewed changes

tannewt merged commit 6aa311a into adafruit:master Aug 22, 2019


		displayio_display_fill_area(self, &area, mask, buffer);

		mp_obj_array_t result = array_new(BYTEARRAY_TYPECODE, buffer_size 4);

	//\| Extract the pixels fro a single row
	//\| Extract the pixels from a single row


		displayio_display_fill_area(self, &area, mask, buffer);

		if ((result->len + result->free) >= (buffer_size * 4)) {

	if (result->typecode != BYTEARRAY_TYPECODE) {
	if (bufinfo.typecode != BYTEARRAY_TYPECODE) {

To displayio.Display: expose fill_area and add a property for rotation #2027

To displayio.Display: expose fill_area and add a property for rotation #2027

Uh oh!

Conversation

dastels commented Jul 31, 2019

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dastels commented Aug 7, 2019

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dastels commented Aug 21, 2019

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dastels commented Aug 22, 2019

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants