Skip to content

Conversation

@kmatch98
Copy link

@kmatch98 kmatch98 commented Feb 10, 2021

A few questions for feedback:

  • A better name of the function Bitmap.blitfancy, maybe blit_rotate_scale?
  • Should the source and destination translation points ox,oy and px,py be sent as (x,y) tuples?
  • Should I add a function to decode two ints from a tuple, since this code is duplicated? Or is this function already available? (save space)
  • Should I add a constrain_clip_boundary function to constrain and arrange the clip coordinates, since this code is duplicated? (Save space)

This version added about 2kB to a build on the PyPortal (atmel-samd). Not sure if it's worth the added space.


Benchmark

Performance of this core function is ~50x faster at blitting than running the same algorithm in Python (tested on PyPortal).

ezgif com-video-to-gif


Here's the Python code for comparison:

# # Copyright (c) 2020 Kevin Matocha <ksmatocha@gmail.com> under same license as below.
# *
# * Credit from here:
# * Copyright (c) 2017 Werner Stoop <wstoop@gmail.com>
# *
# * Permission is hereby granted, free of charge, to any person obtaining a copy
# * of this software and associated documentation files (the "Software"), to deal
# * in the Software without restriction, including without limitation the rights
# * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
# * copies of the Software, and to permit persons to whom the Software is
# * furnished to do so, subject to the following conditions:
# *
# * The above copyright notice and this permission notice shall be included in all
# * copies or substantial portions of the Software.
# *
# * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
# * SOFTWARE.


# Credit from https://github.com/wernsey/bitmap
# MIT License from
#  * Copyright (c) 2017 Werner Stoop <wstoop@gmail.com>
#
# /**
# * #### `void bm_rotate_blit(Bitmap *dst, int ox, int oy, Bitmap *src, int px, int py, double angle, double scale);`
# *
# * Rotates a source bitmap `src` around a pivot point `px,py` and blits it onto a destination bitmap `dst`.
# *
# * The bitmap is positioned such that the point `px,py` on the source is at the offset `ox,oy` on the destination.
# *
# * The `angle` is clockwise, in radians. The bitmap is also scaled by the factor `scale`.
# */
# void bm_rotate_blit(Bitmap *dst, int ox, int oy, Bitmap *src, int px, int py, double angle, double scale);


# 	  /*
#    Reference:
#    "Fast Bitmap Rotation and Scaling" By Steven Mortimer, Dr Dobbs' Journal, July 01, 2001
#    http://www.drdobbs.com/architecture-and-design/fast-bitmap-rotation-and-scaling/184416337
#    See also http://www.efg2.com/Lab/ImageProcessing/RotateScanline.htm
#    */

import math
import time


def blit_rotate_scale(
    destination,  # destination bitmap
    ox=None,
    oy=None,  # (ox, oy) is the destination point where the source (px,py) is placed
    dest_clip0=None,
    dest_clip1=None,  # clip0,1 is (x,y) corners of clip window on the destination bitmap
    source=None,  # source bitmap
    px=None,
    py=None,  # (px, py) is the rotation point of the source bitmap
    source_clip0=None,
    source_clip1=None,  # clip0,1 is (x,y) corners of clip window on the source bitmap
    angle=0,  # in radians, clockwise
    scale=1.0,  # scale factor (float)
    skip_index=None,  # color index to ignore
):
    if source is None:
        pass


# Check the input limits

    if ox is None:
        ox = destination.width / 2
    if oy is None:
        oy = destination.height / 2
    if px is None:
        px = source.width / 2
    if py is None:
        py = source.height / 2

    if dest_clip0 is None:
        dest_clip0 = (0, 0)
    if dest_clip1 is None:
        dest_clip1 = (destination.width, destination.height)

    if source_clip0 is None:
        source_clip0 = (0, 0)
    if source_clip1 is None:
        source_clip1 = (source.width, source.height)

    minx = dest_clip1[0]
    miny = dest_clip1[1]
    maxx = dest_clip0[0]
    maxy = dest_clip0[1]

    sinAngle = math.sin(angle)
    cosAngle = math.cos(angle)

    dx = -cosAngle * px * scale + sinAngle * py * scale + ox
    dy = -sinAngle * px * scale - cosAngle * py * scale + oy
    if dx < minx:
        minx = int(round(dx))
    if dx > maxx:
        maxx = int(round(dx))
    if dy < miny:
        miny = int(round(dy))
    if dy > maxy:
        maxy = int(dy)
    dx = cosAngle * (source.width - px) * scale + sinAngle * py * scale + ox
    dy = sinAngle * (source.width - px) * scale - cosAngle * py * scale + oy
    if dx < minx:
        minx = int(round(dx))
    if dx > maxx:
        maxx = int(round(dx))
    if dy < miny:
        miny = int(round(dy))
    if dy > maxy:
        maxy = int(round(dy))

    dx = (
        cosAngle * (source.width - px) * scale
        - sinAngle * (source.height - py) * scale
        + ox
    )
    dy = (
        sinAngle * (source.width - px) * scale
        + cosAngle * (source.height - py) * scale
        + oy
    )
    if dx < minx:
        minx = int(round(dx))
    if dx > maxx:
        maxx = int(round(dx))
    if dy < miny:
        miny = int(round(dy))
    if dy > maxy:
        maxy = int(round(dy))

    dx = -cosAngle * px * scale - sinAngle * (source.height - py) * scale + ox
    dy = -sinAngle * px * scale + cosAngle * (source.height - py) * scale + oy
    if dx < minx:
        minx = int(round(dx))
    if dx > maxx:
        maxx = int(round(dx))
    if dy < miny:
        miny = int(round(dy))
    if dy > maxy:
        maxy = int(round(dy))

    # /* Clipping */
    if minx < dest_clip0[0]:
        minx = dest_clip0[0]
    if maxx > dest_clip1[0] - 1:
        maxx = dest_clip1[0] - 1
    if miny < dest_clip0[1]:
        miny = dest_clip0[1]
    if maxy > dest_clip1[1] - 1:
        maxy = dest_clip1[1] - 1

    dvCol = math.cos(angle) / scale
    duCol = math.sin(angle) / scale

    duRow = dvCol
    dvRow = -duCol

    startu = px - (ox * dvCol + oy * duCol)
    startv = py - (ox * dvRow + oy * duRow)

    rowu = startu + miny * duCol
    rowv = startv + miny * dvCol

    for y in range(miny, maxy + 1):  # (y = miny, y <= maxy, y++)
        u = rowu + minx * duRow
        v = rowv + minx * dvRow
        for x in range(minx, maxx + 1):  # (x = minx, x <= maxx, x++)
            if (source_clip0[0] <= u < source_clip1[0]) and (
                source_clip0[1] <= v < source_clip1[1]
            ):
                # get the pixel color (c) from the source bitmap at (u,v)
                c = source[
                    int(u) + source.width * int(v)
                ]  # direct index into bitmap is faster than tuple
                # c = source[int(u), int(v)]

                if c != skip_index:  # ignore any pixels with skip_index
                    # place the pixel color (c) into the destination bitmap at (x,y)
                    destination[
                        x + y * destination.width
                    ] = c  # direct index into bitmap is faster than tuple
                    # destination[x,y] = c
            u += duRow
            v += dvRow

        rowu += duCol
        rowv += dvCol

@ladyada
Copy link
Member

ladyada commented Feb 10, 2021

nifty! thanks for the contribution!

@tannewt tannewt self-requested a review February 11, 2021 20:29
@tannewt
Copy link
Member

tannewt commented Feb 11, 2021

A few questions for feedback:

* A better name of the function `Bitmap.blitfancy`, maybe `blit_rotate_scale`?

I'm ok calling it fancy blit.

* Should the source and destination translation points `ox`,`oy` and `px`,`py` be sent as `(x,y)` tuples?

Whatever is consistent with existing APIs (I can't remember.) One reason to avoid tuples is it forces an object creation.

* Should I add a function to decode two ints from a tuple, since this code is duplicated? Or is this function already available? (save space)

Either way. It'll probably be a bit cleaner in the long run.

* Should I add a `constrain_clip_boundary` function to constrain and arrange the clip coordinates, since this code is duplicated? (Save space)

Sure, that'd make sure the implementation is the same.

This version added about 2kB to a build on the PyPortal (atmel-samd). Not sure if it's worth the added space.

I think it's very cool and worth having in CP. I don't think adding it to Bitmap is good though because it increases the size of the baseline Bitmap class. Instead we could have a fancyblit module with functions that take in a bitmap object. We could also generalize the module name if you have other low level bitmap manipulations you want to add.

@kmatch98
Copy link
Author

Thanks for the feedback, I added a few functions to reduce the repetition, saved a little space, but still about 2k larger than the previous.

Makes sense to put it into a different module, since I can envision other bitmap manipulation functions (I'm already thinking about a blit version with color re-mapping options). How about a name like like bitmap_tools?

Next question, is how do I break it into a separate chunk?
Keep it inside displayio, or up at the same level as displayio, or some other way? If you have an example to follow on adding another chunk like this, let me know.

@tannewt
Copy link
Member

tannewt commented Feb 12, 2021

I like bitmap_tools. It'd be a peer of displayio so you'd make a new folder in shared-bindings and common-hal. Since it'd just be functions, you implement them in __init__.c. I think supervisor does something similar.

@deshipu
Copy link

deshipu commented Feb 13, 2021

The "rotate-scale" transform is usually called "rotozoom" in the demoscene — just in case you needed a shorter name.

@kmatch98
Copy link
Author

@tannewt I see the basic idea of what you are talking about for adding a new directory for bitmap_tools in shared-bindings and then for each port in ports\common-hal, but I don't understand the specifics to pull it all together.

Right now I don't forsee any port-specific code, so does it need anything in common-hal for each port?

How do I tell the build system to include the new files when I build a given port?

Thanks for you guidance!

@tannewt
Copy link
Member

tannewt commented Feb 17, 2021

Right now I don't forsee any port-specific code, so does it need anything in common-hal for each port?

Nope! You can just put it in shared-module then.

How do I tell the build system to include the new files when I build a given port?

Take a look in py/circuitpy-* files. There is a regular pattern to new modules there.

@gamblor21
Copy link
Member

@tannewt I see the basic idea of what you are talking about for adding a new directory for bitmap_tools in shared-bindings and then for each port in ports\common-hal, but I don't understand the specifics to pull it all together.

adafruit_bus_device was similarly python code moved to the core so that may help you see what for something similar. Feel free to ping me on discord too as someone who just recently did this from a newbie point of view.

@kmatch98
Copy link
Author

I'm going to close this PR and start a cleaner one with the new module addition.

@kmatch98 kmatch98 closed this Feb 24, 2021
@kmatch98 kmatch98 mentioned this pull request Feb 24, 2021
@kmatch98
Copy link
Author

This PR is superceded by #4256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants