-
Notifications
You must be signed in to change notification settings - Fork 99
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
feat: Add ThorVG build option (IEC-155) #364
Conversation
thorvg/Kconfig
Outdated
|
||
config THORVG_THREAD_SUPPORT | ||
bool "Enable ThorVG thread support" | ||
default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously, we've enabled this feature by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah,lottie, svg, tvg is enabled by default. I'll enable it.
And if we enable the thread
flag, some lottie file will crash, I can't find the reason now, so I set it disable by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If enabling thread flag doesn't work (i.e. it results in a crash), why do we want to keep this as an option? Wouldn't it be better to disable this option, and re-enable it when the bug is fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@espressif2022 Now that you don't get a crash when using threads, can we change this to default y
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@espressif2022 Now that you don't get a crash when using threads, can we change this to
default y
?
Done.
@espressif2022 I have had to change thorvg CMakeLists a lot in this commit: 83ac6d6, it has resolved the PIC issue, details are in the description. Could you please rebase your changes upon the new version? Sorry for the trouble. For pthread support, could you please try it again? Since PIC is now properly disabled, I guess it shouldn't crash anymore. |
Thank you very much, I will deal with it ASAP. |
I've test it. the flags of |
41d8a08
to
870c49e
Compare
thorvg/Kconfig
Outdated
bool "Enable webp loader support" | ||
default n | ||
help | ||
Enable WEBP loader support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally if you don't have more to say in the help text than in the option name, you don't need to add help text... Repeating the same thing twice doesn't really explain anything better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, thanks.
We also get a build failure now:
I guess it's the same as jcmvbkbc/gcc-xtensa#14. Please try adding |
string replace to list Co-authored-by: Ivan Grokhotkov <[email protected]>
thread->threading support Co-authored-by: Ivan Grokhotkov <[email protected]>
update description. Co-authored-by: Ivan Grokhotkov <[email protected]>
Checklist
url
field definedChange description
1: Add Loaders Support option:['tvg', 'svg', 'png', 'jpg', 'lottie']
2: Add log_enable/thread_enable flag