Skip to content

Comments

Optimize the CopyTo method of SKBitmap#3489

Open
jyswjjgdwtdtj wants to merge 3 commits intomono:mainfrom
jyswjjgdwtdtj:main
Open

Optimize the CopyTo method of SKBitmap#3489
jyswjjgdwtdtj wants to merge 3 commits intomono:mainfrom
jyswjjgdwtdtj:main

Conversation

@jyswjjgdwtdtj
Copy link

Description of Change

A simple PR

Optimize the CopyTo method of SKBitmap.

Use direct memory copy instead of creating SKCanvas and DrawBitmap when source and destination Bitmap have the same SKColorType and Width.

Bugs Fixed

None

API Changes

None.

Behavioral Changes

None.

Required skia PR

None.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

@jyswjjgdwtdtj
Copy link
Author

@dotnet-policy-service agree

@jyswjjgdwtdtj
Copy link
Author

请阅读以下贡献者许可协议(CLA)。如果您同意CLA的观点,请回复以下信息。

@dotnet-policy-service agree [company="{your company}"]

选项:

  • (默认 - 未指定公司)我拥有我提交的知识产权独占权,且我不在工作过程中为雇主提交。
@dotnet-policy-service agree
  • (当有人陪伴时)我正在为雇主工作期间提交材料(或者我的雇主根据合同或适用法律拥有我的提交内容的知识产权)。我已获得雇主的许可,可以代表雇主提交意见并签署本协议。在下方签名,定义的“你”一词包括我和我的雇主。
@dotnet-policy-service agree company="Microsoft"

贡献者许可协议

@dotnet-policy-service agree

@jeremy-visionaid
Copy link
Contributor

I also have a memory copy approach since CopyTo is both slow and leaky as is (#2915). Unfortunately, this one doesn't handle that the bitmap might be a subset of another, in which case the source pixel span can be a lot bigger than the destination. I don't mind putting in a PR for mine or helping fix this if it's of interest though?

@jyswjjgdwtdtj
Copy link
Author

I also have a memory copy approach since CopyTo is both slow and leaky as is (#2915). Unfortunately, this one doesn't handle that the bitmap might be a subset of another, in which case the source pixel span can be a lot bigger than the destination. I don't mind putting in a PR for mine or helping fix this if it's of interest though?

you are right. Actually I'm not a Skia professor😢 and I have never used ExtractSubset. I just find my project use up 1gb memories for no reason but a copyto.
May be I should have some additional code to deal with the subset situation.

@jeremy-visionaid
Copy link
Contributor

No worries. I think a fair few people have been tripped up by the leak there. I have a PR to fix that: #3317 You can just copy the code from there for that if you want, but a memory copy like what you were doing is orders of magnitude faster 👍

@jyswjjgdwtdtj
Copy link
Author

jyswjjgdwtdtj commented Feb 2, 2026

No worries. I think a fair few people have been tripped up by the leak there. I have a PR to fix that: #3317 You can just copy the code from there for that if you want, but a memory copy like what you were doing is orders of magnitude faster 👍

It seems that there isn't a property refers to if a skbitmap is a subset of the other one. I don't know whether it's available in skia (not skiasharp )

So maybe the easiest way to solve the problem is to delete ExtractSubset or let the method create a new skbitmap with new memory.

It is not worthwhile to reduce the security and efficiency of the commonly used CopyTo method at the cost of improving the efficiency of the infrequently used ExtractSubset method.

And I think since the name of the method is CopyTo, it must be designed to copy a bitmap to the other one which has the same color type and size. The current CopyTo must be called "DrawTo". This will only lead to ambiguity and inefficiency

It is meaningless to blindly conform to Skia's api

Copy link
Author

@jyswjjgdwtdtj jyswjjgdwtdtj left a comment

Choose a reason for hiding this comment

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

Test if the skbitmap is a subset of another bitmap

@jyswjjgdwtdtj
Copy link
Author

jyswjjgdwtdtj commented Feb 2, 2026

No worries. I think a fair few people have been tripped up by the leak there. I have a PR to fix that: #3317 You can just copy the code from there for that if you want, but a memory copy like what you were doing is orders of magnitude faster 👍

I add some code, and in it I copy pixels row by row when one or both of the src and des bitmap is a subset of another bitmap.
When their ColorType are not the same, CopyTo may still use drawpaint

mattleibow added a commit that referenced this pull request Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants