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

lottie: Support Device Pixel Ratio #31

Merged
merged 3 commits into from
Jul 5, 2024
Merged

lottie: Support Device Pixel Ratio #31

merged 3 commits into from
Jul 5, 2024

Conversation

tinyjin
Copy link
Member

@tinyjin tinyjin commented Jun 23, 2024

Support DPR scaling as a browser scale factor.

Issue: #14

[Before in x300 scale]
CleanShot 2024-06-23 at 17 24 29@2x

[After in x300 scale]
CleanShot 2024-06-23 at 17 23 58@2x

Support DPR scaling as a browser scale factor.

Issue: #14
@tinyjin tinyjin added the enhancement Improve features label Jun 23, 2024
@tinyjin tinyjin requested a review from hermet June 23, 2024 08:25
@tinyjin tinyjin self-assigned this Jun 23, 2024
Copy link

vercel bot commented Jun 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
thorvg-perf-test ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2024 6:57am

@hermet
Copy link
Member

hermet commented Jun 24, 2024

@tinyjin thanks, is it easy to apply DPR skottie?

@tinyjin
Copy link
Member Author

tinyjin commented Jun 24, 2024

@hermet I guess not so difficult. I'll bring it on skottie as well. Should I include skottie DPR in this PR?

@hermet
Copy link
Member

hermet commented Jun 24, 2024

if it's not hard, that would be nice!

@tinyjin
Copy link
Member Author

tinyjin commented Jun 24, 2024

Got it. WIl do soon!

@tinyjin
Copy link
Member Author

tinyjin commented Jun 25, 2024

Want to have DPR as option for each player. However just introduced devicePixelRatio over RenderConfig isn't delivered to NPM package yet. I'll add DPR option toggling on perf-test after coming release with this update

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 consider this option? I don't see the reason we

src/lottie-player.ts Outdated Show resolved Hide resolved
Introduce renderConfig property to have devicePixelRatio as an optional value.
let initialized = false;
let changingRatio = false;
Copy link
Member

Choose a reason for hiding this comment

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

this looks unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, Yes it works. But to change DPR with canvasKit, calling MakeSWCanvasSurface is required after DPR changed, this call takes seconds to process them.

So I define this status to avoid duplicated calling while canvas is built. Because drawFrame is called every frame, I use this condition for the duplicated processing.

if (dpr !== ratio && !changingRatio) {

Copy link
Member

Choose a reason for hiding this comment

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

But your logic looks wiered. changingRatio is only toggled in the scope? Is it correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

drawFrame() is parallelly called. When MakeSWCanvasSurface hangs on, another drawFrame() newly called.

If changingRatio is true, we can skip these process in newly called callback.

Copy link
Member

Choose a reason for hiding this comment

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

it sounds like the context can be broken. should be never happend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we don't need this logic. We just use system default value for DPR. Don't need to think of change in time.

perf-test/components/SkottiePlayer/index.tsx Outdated Show resolved Hide resolved
perf-test/components/SkottiePlayer/index.tsx Outdated Show resolved Hide resolved
@tinyjin tinyjin requested a review from hermet July 5, 2024 07:01
@hermet hermet merged commit c1ee351 into main Jul 5, 2024
2 checks passed
@hermet hermet deleted the jinny/dpr branch July 5, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants