RDKEMW-14869: VideoOutput implementation + fix broken unit tests

[brendanobra]

RDKEMW-21295: Add VideoOutput module to Firebolt C++ Client

Summary

Adds the VideoOutput module to the C++ SDK, providing client-side support for all VideoOutput APIs defined in the Firebolt 9 specification (methods 80–88).

What's included

Generated module files (via fb-gen --emit sync-plan-cpp):

• include/firebolt/videooutput.h — interface, enums, structs
• src/videooutput_impl.h / src/videooutput_impl.cpp — JSON-RPC implementation
• src/json_types/videooutput.h — nlohmann_json serialization
• test/unit/videooutputGeneratedTest.cpp — unit tests (4 cases, all passing)
• test/component/videooutputGeneratedTest.cpp — component test stubs

Accessor wiring (manual):

• include/firebolt/firebolt.h — added #include + VideooutputInterface() virtual method
• src/firebolt.cpp — added impl include, initializer, interface override, unsubscribeAll call, member field

Pre-existing test fixture fixes (unrelated to VideoOutput, fixed broken tests on develop):

• docs/openrpc/the-spec/firebolt-open-rpc.json:
  • Added missing Device.dolbyAtmosExperienceAvailable + event method entry
  • Added missing Localization.timeZone + event method entry
  • Renamed *KiB fields → non-suffixed names in Stats.memoryUsage example to match C++ struct

APIs added

Method	Type	Subscribable
resolution	getter → {width, height}	✓ (onResolutionChanged)
hdcp	getter → enum {hdcp1.4, hdcp2.2, none, direct}	✓ (onHdcpChanged)
cecState	getter → enum {active, inactive, unsupported}	✓ (onCecStateChanged)
refreshRate	getter → enum {0, 23.976, 24, 25, 29.97, 30, 50, 59.94, 60}	✓ (onRefreshRateChanged)
colorDepth	getter → enum {0, 8, 10, 12}	—
colorFormat	getter → enum {ycbcr420, ycbcr422, ycbcr444, rgb444, none}	—
colorimetry	getter → enum {bt2020rgb, bt2020ycc, bt709, oprgb, none}	—
dynamicRange	getter → enum {hdr10, hdr10plus, dolbyVision, hlg, sdr, none}	—
quantizationRange	getter → enum {limited, full, none}	—

Testing

• Unit tests: 111/111 pass (including 4 new VideoOutput tests)
• Library builds clean: libFireboltClient.so links without errors
• Pre-existing test failures (3) fixed by OpenRPC fixture corrections

Spec source

Smithy IDL: firebolt-apis/src/smithy/videooutput.smithy (on feat/smithy branch, not part of this PR)

[Copilot]

Pull request overview

Adds a new (auto-generated) VideoOutput-related module implementation to the Firebolt C++ client, wires it into the IFireboltAccessor singleton, and includes some small formatting/linting changes.

Changes:

• Introduces the videooutput public interface, JSON adapters, and JSON-RPC implementation (include/, src/, src/json_types/).
• Wires the new interface into IFireboltAccessor and FireboltAccessorImpl (include/firebolt/firebolt.h, src/firebolt.cpp).
• Adds initial unit/component test stubs for the module and updates lint/clang-tidy configuration.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
test/unit/videooutputGeneratedTest.cpp	Adds basic generated unit tests (currently minimal coverage).
test/unit/actionsTest.cpp	Formatting-only adjustment to a test call site.
test/component/videooutputGeneratedTest.cpp	Adds generated component-test stubs (compile-time surface checks only).
test/api_test_app/apis/actionsDemo.cpp	Formatting-only output wrapping changes.
src/videooutput_impl.h	Adds VideooutputImpl class declaration and subscription plumbing.
src/videooutput_impl.cpp	Implements Videooutput JSON-RPC calls and subscriptions.
src/json_types/videooutput.h	Adds JSON (de)serialization for Videooutput enums/structs.
src/firebolt.cpp	Wires VideooutputImpl into the accessor singleton and unsubscribeAll flow.
lint.sh	Runs clang-tidy in parallel via run-clang-tidy when available.
include/firebolt/videooutput.h	Adds the new public Videooutput interface/types/method availability helpers.
include/firebolt/firebolt.h	Exposes VideooutputInterface() on IFireboltAccessor.
include/firebolt/actions.h	Formatting-only signature wrapping.
.clang-tidy	Adds clang-tidy configuration.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (6)

include/firebolt/videooutput.h:35

• The new public API uses the module/type spelling Videooutput (e.g., namespace Firebolt::Videooutput, IVideooutput). Existing multi-word modules use UpperCamelCase in the namespace/type names (e.g., namespace Firebolt::TextToSpeech in include/firebolt/texttospeech.h:27). Renaming/regenerating this module to VideoOutput (e.g., Firebolt::VideoOutput::IVideoOutput, VideoOutputImpl, VideoOutputInterface()) would keep the API consistent across modules and align with the PR description’s “VideoOutput”.

namespace Firebolt::Videooutput
{
enum class CecStateValue
{
    Active,

src/videooutput_impl.h:37

• VideooutputImpl deletes copy operations but not move operations. These impl classes hold a IHelper& reference and a SubscriptionManager, so implicit moves can lead to surprising aliasing or invalid subscription state; other modules typically treat impls as non-movable.

    explicit VideooutputImpl(Firebolt::Helpers::IHelper& helper);
    VideooutputImpl(const VideooutputImpl&) = delete;
    VideooutputImpl& operator=(const VideooutputImpl&) = delete;
    ~VideooutputImpl() override = default;

src/videooutput_impl.cpp:26

• <regex> is included but not used in this translation unit, which adds compile-time overhead and unnecessary dependencies.

#include <firebolt/json_types.h>
#include <nlohmann/json.hpp>
#include <regex>

test/component/videooutputGeneratedTest.cpp:36

• The component test only checks that the interface has methods via pointer-to-member, but does not validate any runtime behavior (getter values, subscription delivery, unsubscribe paths). This leaves the new VideoOutput/Videooutput implementation effectively untested at the component level.

TEST(VideooutputGeneratedCTest, InterfaceSurfaceHasresolution)
{
    using Interface = Firebolt::Videooutput::IVideooutput;
    auto ptr = &Interface::resolution;
    (void)ptr;
    SUCCEED();
}

TEST(VideooutputGeneratedCTest, InterfaceSurfaceHascolorDepth)
{
    using Interface = Firebolt::Videooutput::IVideooutput;
    auto ptr = &Interface::colorDepth;
    (void)ptr;
    SUCCEED();
}

include/firebolt/firebolt.h:174

• The new VideooutputInterface() accessor is missing the Doxygen-style comment block that the other interface accessors in this header have, which makes the public API docs inconsistent.

    virtual Videooutput::IVideooutput& VideooutputInterface() = 0;

src/json_types/videooutput.h:103

• The JsonData::*Enum maps are defined but never referenced (e.g., CecStateValueEnum is only defined here). In other modules, these maps are used by a small NL_Json_Basic<Enum> adapter (see DeviceClassJson in src/json_types/device.h:35–39). Either add the corresponding adapter classes and use them from the impl, or remove the unused maps to avoid dead/duplicated enum wiring.

inline const Firebolt::JSON::EnumType<::Firebolt::Videooutput::CecStateValue> CecStateValueEnum({
    {"ACTIVE", ::Firebolt::Videooutput::CecStateValue::Active},
    {"INACTIVE", ::Firebolt::Videooutput::CecStateValue::Inactive},
    {"UNSUPPORTED", ::Firebolt::Videooutput::CecStateValue::Unsupported},
});

[Copilot]

Pull request overview

Copilot reviewed 57 out of 58 changed files in this pull request and generated no new comments.

Suppressed comments (10)

test/unit/videooutputGeneratedTest.cpp:1

• The new test fixture and test case names use inconsistent casing (e.g., Videooutput vs VideoOutput, Forwardsresolution vs ForwardsResolution). Please rename them to match the module/type naming (VideoOutput) and standard CamelCase test naming for readability and consistency.
  test/unit/videooutputGeneratedTest.cpp:1
• The new test fixture and test case names use inconsistent casing (e.g., Videooutput vs VideoOutput, Forwardsresolution vs ForwardsResolution). Please rename them to match the module/type naming (VideoOutput) and standard CamelCase test naming for readability and consistency.
  test/unit/videooutputGeneratedTest.cpp:1
• The new test fixture and test case names use inconsistent casing (e.g., Videooutput vs VideoOutput, Forwardsresolution vs ForwardsResolution). Please rename them to match the module/type naming (VideoOutput) and standard CamelCase test naming for readability and consistency.
  test/component/videooutputGeneratedTest.cpp:1
• The test names use inconsistent casing (Videooutput, Hasresolution, HascolorDepth). Please align with existing naming conventions (e.g., VideoOutputGeneratedCTest and InterfaceSurfaceHasResolution / InterfaceSurfaceHasColorDepth) to keep generated tests easy to scan and search.
  test/component/videooutputGeneratedTest.cpp:1
• The test names use inconsistent casing (Videooutput, Hasresolution, HascolorDepth). Please align with existing naming conventions (e.g., VideoOutputGeneratedCTest and InterfaceSurfaceHasResolution / InterfaceSurfaceHasColorDepth) to keep generated tests easy to scan and search.
  src/videooutput_impl.cpp:1
• These includes appear unused in this translation unit (<regex> and <nlohmann/json.hpp>). Please remove unused/redundant includes to reduce compile time and keep dependencies minimal (the JSON header is already pulled in by json_types/videooutput.h).
  src/json_types/videooutput.h:1
• The thrown error message is too generic for debugging (it doesn't indicate which type/fields are missing). Consider including the object name and the required field list (or the specific missing fields) in the exception message so failures are actionable when surfaced from JSON parsing.
  src/videooutput_impl.h:1
• Only resolution(), colorDepth(), and unsubscribe() are covered by the new unit tests, but this PR introduces several additional getters and subscription APIs. Please add unit tests that validate forwarding/error-propagation for hdcp(), cecState(), refreshRate(), colorFormat(), colorimetry(), dynamicRange(), quantizationRange(), plus at least one subscription path to ensure the correct event method name is used.
  include/firebolt/firebolt.h:174
• The newly added VideoOutputInterface() accessor lacks a Doxygen comment block, while neighboring interface accessors are documented. Please add a brief @brief / @return comment for consistency and to keep the public header documentation complete.

    virtual Actions::IActions& ActionsInterface() = 0;

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

lint.sh:181

• Running run-clang-tidy with -j $(nproc) can overwhelm resource-constrained CI runners (high memory usage / contention), making lint flaky. Consider capping jobs (e.g., min(NPROC, total_files) and/or a conservative max) or allowing an environment variable override (e.g., CLANG_TIDY_JOBS) so CI can tune parallelism.

  NPROC=$(nproc 2>/dev/null || echo 4)

  if [[ "$APPLY_FIXES" == false ]] && command -v run-clang-tidy >/dev/null 2>&1; then
    echo "[lint][clang-tidy] Running ${total_files} files in parallel (${NPROC} jobs)"
    if ! run-clang-tidy -p "$BUILD_DIR" -j "$NPROC" "${source_files[@]}"; then

[Copilot]

Pull request overview

Copilot reviewed 57 out of 58 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

src/json_types/videooutput.h:1

• The serialized wire values don’t match the Firebolt 9 spec described in the PR (e.g., dolbyVision, hdcp1.4, hdcp2.2) and also conflict with the JsonData::*Enum tables later in this file. Update these NLOHMANN_JSON_SERIALIZE_ENUM mappings to use the spec strings (and keep them consistent with the JsonData enum tables) to avoid failed (de)serialization at runtime.
  src/json_types/videooutput.h:1
• The JSON enum table contains a typo: rgbb444 does not match the spec/value used elsewhere (rgb444). This will cause parsing failures if this EnumType is used. Replace rgbb444 with rgb444.
  src/videooutput_impl.cpp:1
• <regex> is included but not used anywhere in this new translation unit as shown. Remove the unused include to reduce compile time and avoid suggesting regex usage where none exists.
  test/unit/videooutputGeneratedTest.cpp:1
• The test fixture name uses Videooutput (lowercase 'o') while the module/type name is VideoOutput. Rename the fixture (and associated TEST_F suite name) to VideoOutputGeneratedUTest to keep naming consistent and improve discoverability in test output.
  include/firebolt/metrics.h:74
• These public virtual method signatures changed from const std::optional<AgePolicy>& to std::optional<AgePolicy> by value. Even though calls may still compile, this is an ABI-breaking change for existing binary consumers of the SDK and forces vtable/signature changes. If ABI stability is required, keep the original parameter types (const reference) and apply [[nodiscard]] without altering argument passing.

    [[nodiscard]] virtual Result<void> startContent(const std::optional<std::string>& entityId,
                                      std::optional<Firebolt::AgePolicy> agePolicy) const = 0;

include/firebolt/metrics.h:86

• These public virtual method signatures changed from const std::optional<AgePolicy>& to std::optional<AgePolicy> by value. Even though calls may still compile, this is an ABI-breaking change for existing binary consumers of the SDK and forces vtable/signature changes. If ABI stability is required, keep the original parameter types (const reference) and apply [[nodiscard]] without altering argument passing.

    [[nodiscard]] virtual Result<void> stopContent(const std::optional<std::string>& entityId,
                                     std::optional<Firebolt::AgePolicy> agePolicy) const = 0;

include/firebolt/firebolt.h:174

• VideoOutputInterface() is added without the Doxygen-style comment block used for the adjacent interface accessors. Add a brief doc comment (matching the surrounding style) so the public accessor API remains consistently documented.

    virtual Actions::IActions& ActionsInterface() = 0;

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

src/json_types/actions.h:56

• The brace style/indentation in this newly edited block is inconsistent and makes the control flow harder to read (closing brace } is mis-indented and not aligned with the if). Reformat this block to match the project's formatting conventions (brace on its own line or consistently indented) to keep generated/maintained code readable.

            if (json["intent"]["context"].contains("source")) {
                ctx.source = json["intent"]["context"]["source"].get<std::string>();
}
            value_.intent.context = ctx;

[Copilot]

Pull request overview

Copilot reviewed 57 out of 58 changed files in this pull request and generated no new comments.

Suppressed comments (14)

src/device_impl.h:38

• This override uses the non-standard POSIX type u_int32_t. The public interface uses uint32_t, and u_int32_t is not available on all platforms/standard libraries. Use uint32_t for portability and consistency.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

src/firebolt.cpp:103

• Keep unsubscribeAll() consistent with the member rename (videoOutput_).

        videooutput_.unsubscribeAll();

src/firebolt.cpp:119

• Keep the member declaration consistent with the module name and the other members in this class by using videoOutput_ (capital O).

    VideoOutput::VideoOutputImpl videooutput_;

test/component/videooutputGeneratedTest.cpp:36

• This component test file only asserts that the interface methods exist (pointer-to-member checks) and does not validate any runtime behavior (getter results, enum decoding, or subscriptions). The repository’s existing module component tests validate against the OpenRPC fixture and/or event delivery (e.g., test/component/deviceTest.cpp). Add component tests that exercise the VideoOutput getters and subscribable events end-to-end.
  test/unit/videooutputGeneratedTest.cpp:61
• The unit tests here only cover construction, unsubscribe forwarding, and transport-error propagation for 2 getters. There are no happy-path tests verifying JSON decoding for resolution() or enum getters, and no tests for the subscription methods (subscribeOn*Changed) or unsubscribeAll(). Add unit tests similar to other modules that validate the decoded return values and subscription wiring using MockHelper/MockBase.
  src/firebolt.cpp:91
• Keep the VideoOutputInterface() accessor consistent with the member rename (videoOutput_).

    VideoOutput::IVideoOutput& VideoOutputInterface() override { return videooutput_; }

src/json_types/videooutput.h:114

• Typo in the enum wire mapping: "rgbb444" does not match the ColorFormatValue::Rgb444 name (and the nlohmann mapping above uses "rgb444"). This would prevent correct parsing if this EnumType map is used (e.g., by future tests via validate_enum).
  src/json_types/videooutput.h:55
• DynamicRangeValue::DolbyVision is serialized as "dolby_vision" here, but elsewhere in this same file (the DynamicRangeValueEnum map) the wire value is "dolbyVision". This inconsistency will break round-tripping and can cause event/getter deserialization to fail depending on which adapter is used. Use a single canonical wire string.
  src/json_types/videooutput.h:66
• HdcpState is serialized as "hdcp14"/"hdcp22" here, but the same file’s HdcpStateEnum uses "hdcp1.4"/"hdcp2.2" (and the PR description also lists dotted values). These should match the actual wire protocol; otherwise deserialization/serialization will not interoperate correctly.
  src/videooutput_impl.cpp:25
• <regex> is included but not used anywhere in this translation unit, which adds unnecessary compile time and dependencies. Remove the unused include.
  src/firebolt.cpp:56
• Member naming in FireboltAccessorImpl is inconsistent with the established lowerCamelCase-with-internal-capitals pattern used elsewhere in this class (e.g., textToSpeech_). Consider renaming videooutput_ to videoOutput_ for consistency.

This issue also appears in the following locations of the same file:

• line 91
• line 103
• line 119

          videooutput_(Firebolt::Helpers::GetHelperInstance())

include/firebolt/firebolt.h:174

• All other interface accessors in this header have a brief Doxygen comment block, but VideoOutputInterface() was added without one. Add a matching comment so the generated API docs stay consistent.

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

include/firebolt/videooutput.h:108

• VideoOutputResolution declares fields as {height, width}, which is inconsistent with the common {width, height} ordering used elsewhere (e.g., Firebolt::Display::DisplaySize in include/firebolt/display.h:27-31) and with the PR description. Because this is an aggregate type, callers using brace-initialization are likely to accidentally swap values. Consider ordering the fields as {width, height}.

struct VideoOutputResolution
{
    uint32_t height;
    uint32_t width;
};

src/json_types/videooutput.h:176

• VideoOutputResolution is an aggregate, so this brace-initialization must match the field order in the public struct. If the struct is {width, height} (consistent with other size structs), this should return {width_, height_} to avoid swapping the values.

[Copilot]

Pull request overview

Copilot reviewed 57 out of 58 changed files in this pull request and generated no new comments.

Suppressed comments (9)

src/device_impl.h:38

• timeInActiveState() is still using the non-standard POSIX typedef u_int32_t. The public interface uses uint32_t, and u_int32_t may be unavailable on non-POSIX toolchains, hurting portability. Align this override to uint32_t.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

test/unit/videooutputGeneratedTest.cpp:47

• The generated unit tests only exercise transport-error forwarding for 2 getters (resolution, colorDepth) plus unsubscribe(). The module exposes 9 getters and 3 subscription APIs, so this leaves most of the new surface untested (including enum deserialization and subscription wiring). Please add at least one happy-path + one transport-error test per getter, and subscribe/unsubscribe coverage for the 3 events.
  test/component/videooutputGeneratedTest.cpp:35
• The component test is currently only a compile-time interface surface check. In this repo, component tests are expected to exercise real JSON-RPC calls via IFireboltAccessor::Instance() and validate against the OpenRPC fixture (including event delivery + negative payload cases where applicable). Please replace/extend these stubs with real component tests for the getters and the 3 subscribable events.
  include/firebolt/firebolt.h:174
• VideoOutputInterface() is the only accessor here without a Doxygen block, which makes the public accessor surface inconsistent and harder to consume in generated docs. Add a brief @brief/@return comment block like the other interfaces.

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

src/stats_impl.h:35

• StatsImpl::~StatsImpl() no longer performs any cleanup (it is = default in the .cpp), so it should be defaulted in the header instead. This avoids an unnecessary out-of-line definition and matches the repo convention for trivial destructors.
  src/lifecycle_impl.h:42
• LifecycleImpl::~LifecycleImpl() no longer performs any cleanup (it is = default in the .cpp), so it should be defaulted in the header instead. This avoids an unnecessary out-of-line definition and matches the repo convention for trivial destructors.
  src/videooutput_impl.cpp:38
• The OpenRPC fixture in docs/openrpc/the-spec/firebolt-open-rpc.json currently has no VideoOutput.* methods/events (e.g. VideoOutput.resolution / VideoOutput.onResolutionChanged). That means the new module can't be validated with the repo’s fixture-driven unit/component test patterns, and the RPC/event names can’t be cross-checked against the spec. Please add the VideoOutput module entries (schemas + examples + events) to the fixture as part of this PR.
  src/videooutput_impl.cpp:64
• Enum getters/subscriptions are implemented via Firebolt::JSON::BasicType<...>, while src/json_types/videooutput.h also defines Firebolt::JSON::EnumType maps for the same enums. This duplicates the wire-value mapping and leaves the *Enum tables unused. To match the pattern used in other modules (e.g. Lifecycle/Device), prefer JsonData adapter classes that decode via EnumType::at() and use those adapters in helper_.get<>() / subscriptionManager_.subscribe<>().
  src/json_types/videooutput.h:44
• This JSON adapter header defines each enum mapping twice: once via NLOHMANN_JSON_SERIALIZE_ENUM(...) and again via Firebolt::JSON::EnumType ...Enum. Since the implementation currently uses BasicType<Enum> (nlohmann-based), the ...Enum tables are unused, and the duplicated sources of truth can drift. Prefer a single enum mapping mechanism (consistent with other modules: Firebolt::JSON::EnumType + JsonData adapter classes) and remove the redundant one.

[Copilot]

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/device_impl.h:38

• timeInActiveState() uses the non-standard POSIX typedef u_int32_t, while the public interface uses uint32_t. Using uint32_t here avoids portability issues on non-POSIX toolchains and keeps the declaration consistent with the interface.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

test/component/videooutputGeneratedTest.cpp:27

• This "component" test file only performs compile-time interface surface checks. Other generated component tests in this repo (e.g., test/component/actionsGeneratedTest.cpp) actually connect via IFireboltAccessor::Instance(), validate getter results, and exercise subscriptions via triggerEvent(...). Add real component tests for VideoOutput getters and the on*Changed events to catch integration/serialization issues.
  src/videooutput_impl.h:20
• This header uses both #pragma once and an #ifndef include guard. Pick one style to avoid redundant/contradictory header protection; the rest of the codebase typically uses only the #ifndef guard for generated impl headers.
  src/json_types/videooutput.h:184
• The namespace-end comment here is incorrect for namespace Firebolt::VideoOutput::JsonData, and to_json(...) is declared at global scope with a stray // namespace Firebolt::VideoOutput comment later. This is confusing and can break ADL-based nlohmann::json serialization (the overload should typically live in the associated namespace). Since this file is auto-generated, this should be corrected in the generator output.

[Copilot]

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated no new comments.

Suppressed comments (12)

src/device_impl.h:38

• timeInActiveState() is declared as Result<u_int32_t> here, but both the public interface (include/firebolt/device.h) and the implementation (src/device_impl.cpp) use uint32_t. Using the non-standard u_int32_t risks portability issues and can break the override on platforms where it differs or isn’t defined.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

test/component/videooutputGeneratedTest.cpp:36

• New module component tests are currently only compile-time interface-surface checks. Existing component tests in this repo validate real JSON-RPC behavior (getters and subscriptions). Please add runtime component tests for the VideoOutput getters/events (e.g., call through IFireboltAccessor::Instance() and validate against the OpenRPC fixture / trigger events).
  test/unit/videooutputGeneratedTest.cpp:28
• The new unit tests only cover unsubscribe() forwarding and transport-error propagation for 2 getters. This leaves most of the new public API (other getters + all subscriptions) without unit coverage, which is inconsistent with the existing module test pattern in this repo.
  src/videooutput_impl.h:20
• This header uses both #pragma once and an include guard. Other generated *_impl.h headers in this repo use the include guard only (e.g. src/actions_impl.h). Mixing both is inconsistent and unnecessary.
  src/json_types/videooutput.h:186
• The JsonData namespace is closed with a misleading comment, and to_json is currently in the global namespace with a closing comment that implies a namespace scope. If to_json is intended for nlohmann ADL, it should live in Firebolt::VideoOutput.
  include/firebolt/firebolt.h:174
• VideoOutputInterface() is missing the Doxygen comment block that all other *Interface() accessors in this header have, which makes the public API documentation inconsistent.

    virtual Actions::IActions& ActionsInterface() = 0;

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

include/firebolt/videooutput.h:38

• Enums in this codebase use SCREAMING_SNAKE_CASE enumerators in the public API (e.g. include/firebolt/lifecycle.h). These new VideoOutput enums introduce PascalCase enumerators (e.g. Active, DolbyVision), which is inconsistent and will leak into all SDK consumers.

enum class CecStateValue
{
    Active,
    Inactive,
    Unsupported,
};

src/stats_impl.h:33

• StatsImpl’s destructor is still declared out-of-line in the header; since it has no custom cleanup it should be defaulted inline (consistent with other *Impl classes) and the .cpp definition removed.
  src/stats_impl.cpp:34
• With ~StatsImpl() defaulted inline in the header, this out-of-line destructor definition should be removed to avoid redundant code.
  src/lifecycle_impl.h:40
• LifecycleImpl’s destructor has no custom cleanup and should be defaulted inline in the header (consistent with other *Impl classes). This also allows removing the .cpp destructor definition.
  src/lifecycle_impl.cpp:35
• With ~LifecycleImpl() defaulted inline in the header, this out-of-line destructor definition should be removed to avoid redundant code.
  src/firebolt.cpp:57
• The member name videooutput_ doesn’t match the established mixed-case pattern used for multi-word modules (e.g. textToSpeech_). Consider renaming to videoOutput_ for consistency and easier grepping.

          textToSpeech_(Firebolt::Helpers::GetHelperInstance()),
          videooutput_(Firebolt::Helpers::GetHelperInstance())
    {

[Copilot]

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated no new comments.

Suppressed comments (7)

src/device_impl.h:38

• timeInActiveState() uses the non-standard u_int32_t typedef. This type is not guaranteed to exist on non-POSIX platforms; use uint32_t for portability and consistency with include/firebolt/device.h.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

test/component/videooutputGeneratedTest.cpp:36

• The new VideoOutput “component tests” only check that a couple of interface methods exist (pointer-to-member). This doesn’t validate runtime JSON-RPC behavior or event delivery the way other component tests do, leaving the new module effectively untested at the integration level.
  src/videooutput_impl.h:20
• This header uses both #pragma once and an include guard. Other module impl headers use only an include guard (e.g., src/actions_impl.h:22-23), so keeping both is redundant and inconsistent.
  src/json_types/videooutput.h:186
• to_json(VideoOutputResolution) is currently defined in the global namespace, so nlohmann's ADL-based serialization may not find it for Firebolt::VideoOutput::VideoOutputResolution. Also the namespace closing comment on the previous brace is inaccurate. Move to_json into namespace Firebolt::VideoOutput and fix the namespace-end comment.
  src/json_types/videooutput.h:36
• NLOHMANN_JSON_SERIALIZE_ENUM(...) is declared inside Firebolt::VideoOutput::JsonData, but the enum types themselves live in Firebolt::VideoOutput. The macro generates to_json/from_json in the current namespace, which means ADL will not find these conversions for the enum types. Align enum (de)serialization with the existing pattern (e.g., src/json_types/device.h) and update VideoOutputImpl to use those adapters.
  include/firebolt/firebolt.h:174
• VideoOutputInterface() is the only accessor method in this interface without a Doxygen block, which breaks the documentation pattern used for all other *Interface() methods in this file.

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

include/firebolt/videooutput.h:142

• The PR description says VideoOutput implements Firebolt 9 APIs (methods 80–88), but the OpenRPC fixture in this repo currently has no VideoOutput entries (no matches for VideoOutput in docs/openrpc/the-spec/firebolt-open-rpc.json). Without fixture/spec updates, the repo’s schema-driven tests can’t validate this new module and capability queries may diverge from the canonical spec.

class IVideoOutput
{
public:
    virtual ~IVideoOutput() = default;

    [[nodiscard]] virtual Result<CecStateValue> cecState() const = 0;
    virtual Result<SubscriptionId> subscribeOnCecStateChanged(std::function<void(const CecStateValue&)>&& notification) = 0;

    [[nodiscard]] virtual Result<ColorDepthValue> colorDepth() const = 0;

    [[nodiscard]] virtual Result<ColorFormatValue> colorFormat() const = 0;

    [[nodiscard]] virtual Result<OutputColorimetry> colorimetry() const = 0;

    [[nodiscard]] virtual Result<DynamicRangeValue> dynamicRange() const = 0;

    [[nodiscard]] virtual Result<HdcpState> hdcp() const = 0;
    virtual Result<SubscriptionId> subscribeOnHdcpChanged(std::function<void(const HdcpState&)>&& notification) = 0;

    [[nodiscard]] virtual Result<QuantizationRangeValue> quantizationRange() const = 0;

    [[nodiscard]] virtual Result<RefreshRateValue> refreshRate() const = 0;
    virtual Result<SubscriptionId>
    subscribeOnRefreshRateChanged(std::function<void(const RefreshRateValue&)>&& notification) = 0;

    [[nodiscard]] virtual Result<VideoOutputResolution> resolution() const = 0;
    virtual Result<SubscriptionId>
    subscribeOnResolutionChanged(std::function<void(const VideoOutputResolution&)>&& notification) = 0;

    virtual Result<void> unsubscribe(SubscriptionId id) = 0;
    virtual void unsubscribeAll() = 0;

}; // class IVideoOutput

[Copilot]

Pull request overview

Copilot reviewed 58 out of 58 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

src/firebolt.cpp:98

• FireboltAccessorImpl::unsubscribeAll() does not forward to device_.unsubscribeAll(), even though Device exposes subscription APIs (e.g., subscribeOnHdrChanged). This can leave active subscriptions registered across Disconnect()/destruction.

    void unsubscribeAll()
    {
        accessibility_.unsubscribeAll();
        actions_.unsubscribeAll();
        lifecycle_.unsubscribeAll();

src/device_impl.h:38

• timeInActiveState() is declared as Result<u_int32_t> here. u_int32_t is a non-standard (POSIX) typedef and the public interface uses uint32_t; using the standard uint32_t improves portability and keeps the interface/impl consistent.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

test/component/videooutputGeneratedTest.cpp:27

• These component tests are compile-time “surface” stubs only (taking pointers to interface methods). This doesn’t exercise the new VideoOutput JSON-RPC wiring end-to-end (getter calls, or subscribeOn* event delivery via triggerEvent + verifyEventReceived), which is the established pattern for component tests in this repo.
  src/videooutput_impl.h:20
• This header uses both #pragma once and an #ifndef/#define include guard. The repo’s generated *_impl.h headers consistently use only the include guard (e.g., src/actions_impl.h), and mixing both is redundant and inconsistent.
  include/firebolt/videooutput.h:33
• PR description states VideoOutput provides client support for all Firebolt 9 VideoOutput APIs and mentions component test stubs, but the OpenRPC fixture in docs/openrpc/the-spec/firebolt-open-rpc.json currently contains no VideoOutput entries at all (no methods/schemas/examples). Without fixture coverage, the usual enum/schema validation and value-based component testing patterns can’t be applied for this module.

namespace Firebolt::VideoOutput
{
enum class CecStateValue

[Copilot]

Pull request overview

Copilot reviewed 58 out of 58 changed files in this pull request and generated 2 comments.

Suppressed comments (9)

src/firebolt.cpp:98

• FireboltAccessorImpl::unsubscribeAll() does not unsubscribe device_, even though Device supports subscriptions. This can leave active subscriptions across Disconnect()/destruction.

    void unsubscribeAll()
    {
        accessibility_.unsubscribeAll();
        actions_.unsubscribeAll();
        lifecycle_.unsubscribeAll();

src/device_impl.h:38

• timeInActiveState() uses the non-standard u_int32_t alias. Use uint32_t to match the public interface (include/firebolt/device.h) and improve portability.

    [[nodiscard]] Result<u_int32_t> timeInActiveState() const override;

test/component/videooutputGeneratedTest.cpp:27

• These are only compile-time interface-surface checks. The new VideoOutput module adds runtime JSON-RPC getters and subscribable events; add component tests that exercise the accessor against the gateway (and verify event delivery) similar to other module component tests.
  src/videooutput_impl.h:20
• This header uses both #pragma once and an include guard. Keep one mechanism to avoid redundant/possibly inconsistent header protection (other generated impl headers use only include guards).
  include/firebolt/firebolt.h:174
• VideoOutputInterface() is missing the Doxygen-style documentation block that the other accessor methods have, which makes the public API docs inconsistent.

    virtual Actions::IActions& ActionsInterface() = 0;

    virtual VideoOutput::IVideoOutput& VideoOutputInterface() = 0;

src/stats_impl.h:35

• This destructor is now defaulted out-of-line in the .cpp; prefer defaulting it in the header and removing the out-of-line definition to reduce churn and keep the class definition self-contained.
  src/lifecycle_impl.h:43
• This destructor is now defaulted out-of-line in the .cpp; prefer defaulting it in the header and removing the out-of-line definition.
  test/unit/videooutputGeneratedTest.cpp:122
• The VideoOutput enum JSON adapters map to string wire values (e.g., "hdcp2.2"), but this test mocks the getter response as an integer. This may not match the real JSON-RPC payload shape and can mask parsing issues.
  src/lifecycle_impl.h:43
• The destructor is defined out-of-line (in the .cpp) but does no custom work. Defaulting it in the header is simpler; if you do that, remove the out-of-line definition in lifecycle_impl.cpp to avoid multiple definitions.