-
Notifications
You must be signed in to change notification settings - Fork 187
media: Update Audio/Video CodecCapabilities for testing #9048
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
|
/gemini review |
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.
Code Review
This pull request refactors CodecCapability and its derived classes to facilitate unit testing by decoupling them from JNI dependencies. This is achieved by introducing CreateForTesting static factory methods and corresponding constructors that don't rely on Java objects. The overall approach is sound and effectively addresses the problem described. My review includes suggestions to adopt modern C++ best practices, specifically using std::make_unique, which aligns with the project's referenced Chromium style guide and improves code safety and readability.
df422a0 to
b409152
Compare
b409152 to
995e0f7
Compare
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.
Code Review
The pull request introduces new constructors and CreateForTesting static methods for CodecCapability, AudioCodecCapability, and VideoCodecCapability classes. These changes aim to facilitate unit testing by decoupling the object creation from JNI calls. Additionally, a null check for j_video_capabilities_ has been added in VideoCodecCapability::AreResolutionAndRateSupported to prevent crashes in unit test environments. The changes are well-explained and address a clear need for improved testability.
| return std::unique_ptr<AudioCodecCapability>(new AudioCodecCapability( | ||
| std::move(name), is_secure_req, is_secure_sup, is_tunnel_req, | ||
| is_tunnel_sup, supported_bitrates)); |
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.
The CreateForTesting method uses new AudioCodecCapability(...) directly. While this is functional, it's generally preferred to use std::make_unique for creating std::unique_ptr instances. std::make_unique provides exception safety and can be more efficient by performing a single allocation for both the object and the unique_ptr's control block.
return std::make_unique<AudioCodecCapability>(std::move(name), is_secure_req,
is_secure_sup, is_tunnel_req,
is_tunnel_sup, supported_bitrates);| return std::unique_ptr<VideoCodecCapability>(new VideoCodecCapability( | ||
| std::move(name), is_secure_req, is_secure_sup, is_tunnel_req, | ||
| is_tunnel_sup, is_software_decoder, is_hdr_capable, | ||
| /*j_video_capabilities=*/std::move(null_ref), supported_widths, | ||
| supported_heights, supported_bitrates, supported_frame_rates)); |
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.
Similar to the AudioCodecCapability::CreateForTesting method, it's recommended to use std::make_unique here for consistency and best practices.
return std::make_unique<VideoCodecCapability>(std::move(name), is_secure_req,
is_secure_sup, is_tunnel_req,
is_tunnel_sup, is_software_decoder,
is_hdr_capable, std::move(null_ref),
supported_widths, supported_heights,
supported_bitrates, supported_frame_rates);Bug: 433773029
995e0f7 to
665deaa
Compare
Currently, creating unittests for MediaCapabilitiesCache::FindVideoDecoder() is difficult due to the function's reliance on VideoCodecCapabilities. This class (along with its sibling class AudioCodecCapabilities and parent class CodecCapabilities) are heavily coupled with JNI calls, which makes mocking them in a unit testing environment difficult.
This PR addresses this issue by updating VideoCodecCapabilities and AudioCodecCapabilities to now be able to create instances of each class for testing purposes. Each class has a new
CreateForTestingstatic method, which will allow developers to create these objects with their own values. These new methods are completely decoupled from JNI initialization, making them suitable for a unit testing environment.The class function VideoCodecCapability::AreResolutionAndRateSupported has been altered slightly to now check if the |j_video_capabilities_| value is not null. In production, this change is no-op as |j_video_capabilities_| is always assigned. However, in a unit testing environment, this value is set to null as we have no means of properly retrieving the corresponding java object.
Follow-up PRs will be added that will make use of these new testing ctors.
Bug: 433773029