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

Support: jpeg decoder on esp32c2 #525

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

vikramdattu
Copy link
Contributor

@vikramdattu vikramdattu commented Mar 31, 2023

Previous PR tried fixing this by just including tjpegd.h rom header. However this is not true for all chip versions of esp32c2. #524

Also, disabled camera function calls for ESP32C2 from example as this is not supported. This helps to run the CI without linking errors.

PR #524 will be closed in favour of this.

@vikramdattu
Copy link
Contributor Author

cc @igrr @me-no-dev

examples/main/take_picture.c Outdated Show resolved Hide resolved
@igrr
Copy link
Member

igrr commented Mar 31, 2023

LGTM as an interim solution, just one comment about the target check.

Long term, it would be best to move the image processing functions into a separate component so that they could be included without the actual camera driver.

@me-no-dev
Copy link
Member

My issue with this PR is that it's specific to just one unsupported chip. Or am I missing something? What would happen on C3/C6/H2, etc?

@vikramdattu
Copy link
Contributor Author

vikramdattu commented Apr 19, 2023

My issue with this PR is that it's specific to just one unsupported chip. Or am I missing something? What would happen on C3/C6/H2, etc?

@me-no-dev you're right. For C3 it's supported in ROM but for the other chips not. How about we do this change (software jpeg decoder) for all the unsupported chips?

.github/workflows/build.yml Outdated Show resolved Hide resolved
@igrr
Copy link
Member

igrr commented Apr 19, 2023

Regarding other chips, i would recommend using CONFIG_ESP_ROM_HAS_JPEG_DECODE option to automatically use the fallback jpeg decoder, instead of relying on CONFIG_IDF_TARGET_X. This way we won't have to update the code for each of the unsupported chips.

@me-no-dev me-no-dev mentioned this pull request Apr 20, 2023
@tore-espressif
Copy link
Contributor

@me-no-dev @igrr We also started with esp_jpeg component https://github.com/espressif/idf-extra-components/tree/master/esp_jpeg

This component will automatically use 'soft' implementation, if the target does not contain the decoder in ROM.
Could you please have a brief look whether it fits your needs?
It could potentially offload esp32-camera maintainers with JPEG ROM issues...

@me-no-dev
Copy link
Member

@me-no-dev @igrr We also started with esp_jpeg component https://github.com/espressif/idf-extra-components/tree/master/esp_jpeg

This component will automatically use 'soft' implementation, if the target does not contain the decoder in ROM. Could you please have a brief look whether it fits your needs? It could potentially offload esp32-camera maintainers with JPEG ROM issues...

Yes. This is the way forward, but let's have it for next version, where more things will change.

@me-no-dev
Copy link
Member

Regarding other chips, i would recommend using CONFIG_ESP_ROM_HAS_JPEG_DECODE option to automatically use the fallback jpeg decoder, instead of relying on CONFIG_IDF_TARGET_X. This way we won't have to update the code for each of the unsupported chips.

CONFIG_ESP_ROM_HAS_JPEG_DECODE exists starting with IDF v5.0. #define ESP_ROM_HAS_JPEG_DECODE exists from IDF v4.3.

We could always build the source, but ifdef inside everything to be compiled only if IDF version is equal or more than 4.3 and ESP_ROM_HAS_JPEG_DECODE is not defined. Header is the same for all chips, so we could just use one

@vikramdattu
Copy link
Contributor Author

Regarding other chips, i would recommend using CONFIG_ESP_ROM_HAS_JPEG_DECODE option to automatically use the fallback jpeg decoder, instead of relying on CONFIG_IDF_TARGET_X. This way we won't have to update the code for each of the unsupported chips.

CONFIG_ESP_ROM_HAS_JPEG_DECODE exists starting with IDF v5.0. #define ESP_ROM_HAS_JPEG_DECODE exists from IDF v4.3.

We could always build the source, but ifdef inside everything to be compiled only if IDF version is equal or more than 4.3 and ESP_ROM_HAS_JPEG_DECODE is not defined. Header is the same for all chips, so we could just use one

@me-no-dev
The only issue I see is that, CMakeList still has to do juggling when CONFIG_ESP_ROM_HAS_JPEG_DECODE is not available. But in source files, using ESP_ROM_HAS_JPEG_DECODE should be okay.

Also, disabled camera_init and capture from example for chips not
supporting camera

Signed-off-by: Vikram <vikram.dattu@espressif.com>
Signed-off-by: Vikram <vikram.dattu@espressif.com>
@me-no-dev me-no-dev merged commit e689c3b into espressif:master Apr 25, 2023
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