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

thorvg: add component of thorvg (IEC-115) #332

Merged

Conversation

espressif2022
Copy link
Contributor

@espressif2022 espressif2022 commented May 20, 2024

Checklist

  • Component contains License
  • Component contains README.md
  • Component contains idf_component.yml file with url field defined
  • Component was added to upload job
  • Component was added to build job
  • Optional: Component contains unit tests
  • CI passing

Change description

1: Add component of thorvg.
2: Example is here .
3: The structure refers to this PR of LVGL.

@github-actions github-actions bot changed the title thorvg: add component of thorvg thorvg: add component of thorvg (IEC-115) May 20, 2024
@espressif2022 espressif2022 force-pushed the feature/add_component_thorvg branch 6 times, most recently from cc5b0e5 to c126b36 Compare May 21, 2024 02:49
@suda-morris
Copy link
Collaborator

@espressif2022 instead of copying the thorvg source code, can we use git submodule?

@suda-morris
Copy link
Collaborator

can we also provide an example for this?

@suda-morris suda-morris requested a review from igrr May 21, 2024 10:16
thorvg/idf_component.yml Outdated Show resolved Hide resolved
@igrr
Copy link
Member

igrr commented May 21, 2024

LGTM overall, aside from the points mentioned above.

Please also add an SBOM file, there is an example in fmt component.

@igrr
Copy link
Member

igrr commented May 21, 2024

Regarding an example, I agree it would be useful. You can do something very simple which only shows the API usage. For instance, see this other PR where I'm adding a FreeType example: #320.

@espressif2022
Copy link
Contributor Author

@espressif2022 instead of copying the thorvg source code, can we use git submodule?

ok, modify this in my fork repository, and then use submodule here.

@espressif2022
Copy link
Contributor Author

Regarding an example, I agree it would be useful. You can do something very simple which only shows the API usage. For instance, see this other PR where I'm adding a FreeType example: #320.

Okay, I'll add an example referring to this.

@igrr
Copy link
Member

igrr commented May 22, 2024

@espressif2022 regarding this comment,

ok, modify this in my fork repository, and then use submodule here.

What changes did you have to make to the upstream project, could you please show a diff? Thorvg can usually be ported to different platforms without modifying its source code, so I wonder if we could do the same here.

@espressif2022
Copy link
Contributor Author

@espressif2022 regarding this comment,

ok, modify this in my fork repository, and then use submodule here.

What changes did you have to make to the upstream project, could you please show a diff? Thorvg can usually be ported to different platforms without modifying its source code, so I wonder if we could do the same here.

Mainly cmake compilation changes, because thorvg is built with meson and should be incompatible with our ESP-IDF.
So here we need to use the original thorvg repository?

@igrr
Copy link
Member

igrr commented May 22, 2024

Yeah, using the original repo is the best option. This will make releasing new thorvg versions simpler, since we will only need to update the submodule pointer.

Can you describe what these "cmake compilation changes" were? As I understand, you have added component CMakeLists.txt, which is outside of thorvg directory. So is there anything extra that needs to be done inside thorvg itself?

@espressif2022
Copy link
Contributor Author

Yeah, using the original repo is the best option. This will make releasing new thorvg versions simpler, since we will only need to update the submodule pointer.

Can you describe what these "cmake compilation changes" were? As I understand, you have added component CMakeLists.txt, which is outside of thorvg directory. So is there anything extra that needs to be done inside thorvg itself?

The internal build system uses Meson: meson.build.
I see that lvgl's third-party library directly changes meson to cmake, which may be simpler.
Is it possible to be compatible with meson here?

Additionally, some internal function names are causing compilation issues(my self-test seems to be fine), as mentioned here: Issue 2062.

@igrr
Copy link
Member

igrr commented May 23, 2024

Both options are okay:

  1. Add the original repo as a submodule, then add component CMakeLists.txt file as you already did in this PR. Since the CMakeLists.txt file is outside of the thorvg submodule directory, you don't need to modify the submodule this way.
  2. Add the original repo as a submodule, then add component CMakeLists.txt file, then use ExternalProject_Add to perform the build of thorvg using meson.

Let me know if you meet any issue or need any help.

@espressif2022
Copy link
Contributor Author

espressif2022 commented May 24, 2024

Both options are okay:

  1. Add the original repo as a submodule, then add component CMakeLists.txt file as you already did in this PR. Since the CMakeLists.txt file is outside of the thorvg submodule directory, you don't need to modify the submodule this way.
  2. Add the original repo as a submodule, then add component CMakeLists.txt file, then use ExternalProject_Add to perform the build of thorvg using meson.

Let me know if you meet any issue or need any help.

  • When I use ExternalProject_Add build thorvg with Meson, I encounter some issues. thorvg will use the system's cross-compilation toolchain.
Project name: thorvg
Project version: 0.13.99
C++ compiler for the host machine: ccache c++ (gcc 11.4.0 "c++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0")
C++ linker for the host machine: c++ ld.bfd 2.38
Host machine cpu family: x86_64
Host machine cpu: x86_64
Configuring config.h using configuration
Library pthread found: YES
Found pkg-config: YES (/usr/bin/pkg-config) 0.29.2
/home/xuxin/.espressif/tools/riscv32-esp-elf/esp-13.2.0_20240305/riscv32-esp-elf/bin/../lib/gcc/riscv32-esp-elf/13.2.0/../../../../riscv32-esp-elf/bin/ld: thorvg_build/src/libthorvg.a: error adding symbols: file in wrong format
collect2: error: ld returned 1 exit status
  • It seems like I need to pass a cross-compilation configuration file. I'm not sure how to implement this in the IDF environment. How should I adjust or where can I find references for this?

@igrr
Copy link
Member

igrr commented May 24, 2024

I think you can use CMake file(WRITE...) command to generate a Meson cross-compilation config file inside the build directory, then pass the path of this file to Meson.

@espressif2022
Copy link
Contributor Author

I think you can use CMake file(WRITE...) command to generate a Meson cross-compilation config file inside the build directory, then pass the path of this file to Meson.

I tested to use a fixed config file .

CONFIGURE_COMMAND meson setup ${THORVG_BUILD_DIR} ${THORVG_SUBDIR} --cross-file ${CROSS_FILE} -Dbindings=capi -Ddefault_library=static

I encountered a strdup error, despite having included cstring in the header file.
I'm not sure if it's an issue with our environment's standard library, I see the cross file example has this.

../../components/espressif__thorvg/thorvg/src/renderer/tvgText.h:65:32: error: 'strdup' was not declared in this scope; did you mean 'strcmp'?
   65 |         if (utf8) this->utf8 = strdup(utf8);

@igrr
Copy link
Member

igrr commented May 24, 2024

Can you try adding -D_GNU_SOURCE to the compilation flags of thorvg? (c_args option in meson)
I think this should make strdup visible: https://github.com/espressif/newlib-esp32/blob/7cdcb0b0848a5d909e906834fe81903ba9466c90/newlib/libc/include/string.h#L83-L85

@espressif2022
Copy link
Contributor Author

@espressif2022 instead of copying the thorvg source code, can we use git submodule?

replaced now.

@espressif2022
Copy link
Contributor Author

can we also provide an example for this?

Added, riscv32 in P4 and xtensa in s3 are tested.

@espressif2022
Copy link
Contributor Author

Can you try adding -D_GNU_SOURCE to the compilation flags of thorvg? (c_args option in meson) I think this should make strdup visible: https://github.com/espressif/newlib-esp32/blob/7cdcb0b0848a5d909e906834fe81903ba9466c90/newlib/libc/include/string.h#L83-L85

Both riscv32 on P4 and xtensa on s3 have been tested.

thorvg/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@suda-morris suda-morris left a comment

Choose a reason for hiding this comment

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

LGTM overall

thorvg/sbom_thorvg.yml Outdated Show resolved Hide resolved

file(APPEND ${TVG_CROSS_CFG} "
[host_machine]
system = 'android'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, why set the host to android?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, why set the host to android?

if get_option('threads') == true and host_machine.system() != 'windows' and host_machine.system() != 'android'
thread_dep = meson.get_compiler('cpp').find_library('pthread')
thorvg_lib_dep += [thread_dep]
endif

To solve the thread_dep error, but later added -Dthreads=false, I think it is better to write freertos here now.

Copy link
Member

Choose a reason for hiding this comment

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

We do provide pthreads library, so enabling thread support should work fine. What was the exact error you got here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do provide pthreads library, so enabling thread support should work fine. What was the exact error you got here?

Project name: thorvg
Project version: 0.13.6
C++ compiler for the host machine: /home/xuxin/.espressif/tools/riscv32-esp-elf/esp-13.2.0_20240530/riscv32-esp-elf/bin/riscv32-esp-elf-g++ (gcc 13.2.0 "riscv32-esp-elf-g++ (crosstool-NG esp-13.2.0_20240530) 13.2.0")
C++ linker for the host machine: /home/xuxin/.espressif/tools/riscv32-esp-elf/esp-13.2.0_20240530/riscv32-esp-elf/bin/riscv32-esp-elf-g++ ld.bfd 13.2.0
C++ compiler for the build machine: ccache c++ (gcc 11.4.0 "c++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0")
C++ linker for the build machine: c++ ld.bfd 2.38
Build machine cpu family: x86_64
Build machine cpu: x86_64
Host machine cpu family: Generic
Host machine cpu: esp
Target machine cpu family: Generic
Target machine cpu: esp
Configuring config.h using configuration

../../../../thorvg/src/meson.build:47:43: ERROR: C++ shared or static library 'pthread' not found

A full log can be found at /home/xuxin/esp_work/idf-extra-components_fork/thorvg/examples/thorvg-example/build/thorvg_build/meson-logs/meson-log.txt

Copy link
Member

Choose a reason for hiding this comment

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

I see, this should be possible to solve by adding pthread component library to the linker flags.

We can keep this as a follow-up, I think it's okay to not fix this in the current MR. (Provided that thorvg is used from one FreeRTOS task only.)

@suda-morris
Copy link
Collaborator

suda-morris commented Jun 3, 2024

@espressif2022 Please fix the CI pipeline and squash the commits before merge.

thorvg/CMakeLists.txt Outdated Show resolved Hide resolved
thorvg/CMakeLists.txt Outdated Show resolved Hide resolved
esp32_p4_function_ev_board_noglib:
version: ">=1.2.0"
rules:
- if: "target == esp32p4"
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this example is used for a different target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens when this example is used for a different target?

It will compile and report an error.
Because PPA has been added here, and only the BSP of esp32_p4_function_ev_board has the bsp_display_new_with_handles function, so the example only supports P4.

@espressif2022 espressif2022 force-pushed the feature/add_component_thorvg branch 6 times, most recently from 3a273fa to 331a7f3 Compare June 5, 2024 13:24
INCLUDE_DIRS "."
PRIV_REQUIRES esp_lcd esp_timer esp_driver_ppa)

idf_component_get_property(board_lib espressif__esp32_p4_function_ev_board_noglib COMPONENT_LIB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@espzav will you fix the build warning?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What warning is there? I will try it on the board tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What warning is there? I will try it on the board tomorrow.

@espzav , this warning.

espressif__esp32_p4_function_ev_board_noglib/esp32_p4_function_ev_board.c:33:31: warning: 'tp' defined but not used [-Wunused-variable]
   33 | static esp_lcd_touch_handle_t tp;   // LCD touch handle

@suda-morris suda-morris merged commit 7473d83 into espressif:master Jun 6, 2024
71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants