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

sw_engine raster: image rastering optimization #239

Conversation

mgrudzinska
Copy link
Collaborator

@mgrudzinska mgrudzinska commented Feb 8, 2021

Adding functions for rastering the translated images (no rotation and scaling).

This pr contains 3 commits for readabilty.

I introduced changes reducing the number of multiplications performed in loops.
I changed the order of the functions, sticking to the rule (for each block): first identity, then for translated images and finally for general transformations.
In the last commit I introduced all functions for rastering the translated images.

Mira Grudzinska added 2 commits February 22, 2021 22:43
The alpha value calculation pulled out outside the inner loop
to reduce the number of unnecessary operations.
Added local variables to reduce the number of costly
multiplications performed in a loop.
To improve the readability of the file.
@mgrudzinska mgrudzinska force-pushed the mgrudzinska/Image_raster_opt_transl branch from 5ce5276 to 3c55b58 Compare February 22, 2021 22:57
Functions for rastering the translated images added
(no rotation or scaling).
@mgrudzinska mgrudzinska force-pushed the mgrudzinska/Image_raster_opt_transl branch from 3c55b58 to 64293a7 Compare February 22, 2021 23:22
@mgrudzinska mgrudzinska requested review from hermet and JSUYA March 4, 2021 11:25
Copy link
Member

@hermet hermet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too much function introduction...
For translated image, I think we can reuse the exiting image drawing method...

@@ -71,17 +71,21 @@ static bool _inverse(const Matrix* transform, Matrix* invM)
}


static bool _identify(const Matrix* transform)
static int _identify(const Matrix* m)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does more than identify() check.... bad.

else return _rasterImage(surface, image->data, image->w, image->h, bbox);
}
//Translation
if (translucent) return _rasterTranslucentImage(surface, image->data, image->w, image->h, opacity, bbox, -transform->e13, -transform->e23);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't reuse the existing method? We can just offset the region xy...
_rasterTranslucentImage(SwSurface* surface, uint32_t *img, uint32_t w, uint32_t h, uint32_t opacity, const SwBBox& region);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my problem is that I can't see how to use a region because both pictures and shapes/svgs may use the same rastering functions. For ex an svg with clipping and opacity will use _translucentImage, and a translucent picture (pixels) uses it as well.
For a picture I can easily use region to call the same function for the identity transform and the translation, but it's not what I want to do for an svg...

Am I missing something?

Copy link
Member

@hermet hermet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't come in the same patch thread unless it has the previous patch dependency...

Copy link
Member

@hermet hermet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please separate PR this?

@mgrudzinska
Copy link
Collaborator Author

@hermet please check #274 - I separated one of the commits.

@mgrudzinska mgrudzinska marked this pull request as draft April 12, 2021 09:22
@hermet
Copy link
Member

hermet commented Nov 15, 2021

#1053

@hermet hermet closed this Nov 15, 2021
@hermet hermet added the invalid This doesn't seem right label Nov 15, 2021
@mgrudzinska mgrudzinska deleted the mgrudzinska/Image_raster_opt_transl branch November 26, 2021 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants