Fix encoding logic #70
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the encoding architecture by removing the direct frame callback mechanism in favor of using a Consumer-based pattern. The changes simplify the API by consolidating frame handling through the Consumer interface.
- Replaces frameCallback with consumer-based architecture using ConsumerToFrameCallback adapter
- Adds validation to ensure consumer is set before encoding operations
- Updates CMake configuration to make encoder dependencies public for better linking
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test_PullToPushSource.cpp | Updates test to use ConsumerToFrameCallback instead of direct frameCallback |
| PullToPushSource.cpp | Adds consumer validation and calls parent startProducing method |
| PushSource.h | Removes frameCallback support, keeping only consumer-based frame handling |
| Consumer.h | Introduces ConsumerToFrameCallback adapter class for backward compatibility |
| test_Encoderx264.cpp | Adds test for missing consumer validation and updates existing test |
| Encoderx264.cpp | Adds consumer validation and removes frameCallback fallback logic |
| Encoder.h | Changes enum to enum class and adds WrongConfiguration error type |
| CMakeLists.txt | Changes encoder library linking from PRIVATE to PUBLIC |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| public: | ||
| enum Type | ||
| enum class Type |
There was a problem hiding this comment.
Changing from 'enum Type' to 'enum class Type' is a breaking change that will require updates to existing code that references these enum values. Consider the impact on existing consumers of this API.
| enum class Type | |
| enum Type |
| if (consumer == nullptr) { | ||
| throw SourceError(SourceError::SOURCE_ERROR_WRONG_CONFIG, "No consumer or frame callback set"); | ||
| } | ||
| PushSource::startProducing(config).get(); |
There was a problem hiding this comment.
Calling .get() on a future blocks the current thread and can cause deadlocks or performance issues. Consider handling the future asynchronously or ensure this blocking behavior is intentional.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
linux/workflow_build.py
Outdated
| subprocess.run( | ||
| ["./Decoders_tests", "-s"], | ||
| cwd="./build/modules/decoders", | ||
| ["./ctest", "-s"], |
There was a problem hiding this comment.
The command ./ctest is incorrect. CTest is typically invoked as ctest (without the ./ prefix) since it's a CMake-provided tool installed in the system path, not a local executable in the build directory. This will fail unless there's a custom ctest wrapper in the build directory.
| ["./ctest", "-s"], | |
| ["ctest", "-s"], |
| startPromise = std::make_unique<std::promise<void>>(); | ||
| stopRequested.store(false); | ||
| workerThread = std::thread([this]() { | ||
| workerThread = std::thread([this, &config]() { |
There was a problem hiding this comment.
Capturing config by reference in the lambda is dangerous because the lambda may outlive the config parameter's lifetime. The ProducingConfiguration object will be destroyed when startProducing returns, but the thread continues running and may access the dangling reference. Capture by value instead: [this, config]() to avoid undefined behavior.
| workerThread = std::thread([this, &config]() { | |
| workerThread = std::thread([this, config]() { |
| { | ||
| public: | ||
| enum Type | ||
| enum class Type |
There was a problem hiding this comment.
Changing from enum Type to enum class Type is a breaking change that requires all existing references to update from EncoderException::Unknown to EncoderException::Type::Unknown. Ensure all call sites throughout the codebase have been updated accordingly, particularly in error handling code that may catch and check exception types.
No description provided.