nojira: fix videoutput ser/deserialization bug(s)

[brendanobra]

Summary

This PR fixes VideoOutput enum marshalling by replacing default enum decoding with explicit custom JSON adapters that correctly parse string wire payloads. The original pr is: #97

Problem

VideoOutput getters and subscriptions failed to parse valid payloads such as hdcp1.4, inactive, ycbcr422, sdr, and limited.

Root Cause

The previous implementation used a default enum marshaller path that did not match the actual wire format (string enums).

Changes

• Added custom VideoOutput enum adapters for:
  • HdcpState
  • CecStateValue
  • RefreshRateValue
  • ColorDepthValue
  • ColorFormatValue
  • OutputColorimetry
  • DynamicRangeValue
  • QuantizationRangeValue
• Updated VideoOutput getter/subscription wiring to use custom adapters instead of default enum marshalling.
• Updated unit tests to validate string wire payloads (instead of enum ordinals).
• Added component coverage for:
  • valid enum string parsing
  • invalid enum value rejection
  • runtime subscriptions:
    • VideoOutput.onHdcpChanged
    • VideoOutput.onCecStateChanged
    • VideoOutput.onRefreshRateChanged
• Updated mock OpenRPC fixture by:
  • adding missing VideoOutput getter/event entries
  • correcting VideoOutput event result schemas to string-enum schemas

Validation

• ./run-unit-tests.sh -> 170 passed
• ./run-component-tests-local.sh --skip-image-build -> 107 passed, 0 failed, 0 skipped

Scope

Server-side alias behavior is out of scope. This PR is focused on client-side marshalling correctness and regression coverage.

[Copilot]

🟡 Changes recommended

The OpenRPC fixture changes include incorrect schema updates applied to unrelated Lifecycle2/TextToSpeech events, and the new VideoOutput component tests currently skip on subscribe errors, which can mask real regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the VideoOutput client to correctly deserialize enum values from wire-format strings by introducing explicit JSON enum adapters, then wires the getters/subscriptions to use those adapters. It also updates unit/component tests and the OpenRPC fixture intended to support/validate the VideoOutput subscription surface in the mock environment.

Changes:

• Added explicit JsonData::*Json adapters for VideoOutput enum types and updated VideoOutputImpl to use them for getters/subscriptions.
• Updated VideoOutput unit tests to use wire-format string payloads (and event payloads) instead of enum ordinals.
• Expanded component tests with runtime subscription assertions and additional marshaller parsing/rejection tests; updated OpenRPC fixture entries for VideoOutput.

File summaries

File	Description
test/unit/videooutputGeneratedTest.cpp	Updates unit tests to expect string enum payloads for VideoOutput getters/events.
test/component/videooutputGeneratedTest.cpp	Adds marshaller tests and runtime subscription tests for VideoOutput enum-string parsing/rejection.
src/videooutput_impl.cpp	Switches VideoOutput getters/subscriptions from default enum marshalling to explicit JSON adapters.
src/json_types/videooutput.h	Introduces explicit JSON adapters that map wire strings to VideoOutput enum values.
docs/openrpc/the-spec/firebolt-open-rpc.json	Adds/adjusts VideoOutput method/event entries and schemas in the OpenRPC fixture used by mock-firebolt.

Review details

Suppressed comments (2)

test/component/videooutputGeneratedTest.cpp:170

• This test skips whenever subscribeOnCecStateChanged returns an error, which can mask real regressions (other component tests always fail via verifyEventSubscription(id) instead of skipping). Removing the skip keeps the test meaningful.

    if (!id)
    {
        GTEST_SKIP() << "VideoOutput.onCecStateChanged is not available in the current mock OpenRPC fixture";
    }
    verifyEventSubscription(id);

test/component/videooutputGeneratedTest.cpp:199

• This test skips whenever subscribeOnRefreshRateChanged returns an error, which can mask real regressions (other component tests always fail via verifyEventSubscription(id) instead of skipping). Removing the skip keeps the test meaningful.

    if (!id)
    {
        GTEST_SKIP() << "VideoOutput.onRefreshRateChanged is not available in the current mock OpenRPC fixture";
    }
    verifyEventSubscription(id);

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The OpenRPC fixture changes incorrectly alter Lifecycle2.onStateChanged and TextToSpeech event schemas to unrelated VideoOutput enums, which needs to be corrected before merging.

Review details

Suppressed comments (3)

docs/openrpc/the-spec/firebolt-open-rpc.json:3450

• The OpenRPC schema for Lifecycle2.onStateChanged was changed to an HDCP enum, which is unrelated to lifecycle and will misdocument/validate lifecycle event payloads.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

docs/openrpc/the-spec/firebolt-open-rpc.json:3504

• The OpenRPC schema for TextToSpeech.onWillspeak was changed to a CEC-state enum, which is unrelated to TextToSpeech and makes this event’s schema incorrect.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

docs/openrpc/the-spec/firebolt-open-rpc.json:3557

• The OpenRPC schema for TextToSpeech.onSpeechstart was changed to a refresh-rate enum, which is unrelated to TextToSpeech and makes this event’s schema incorrect.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

It edits files explicitly marked auto-generated (“DO NOT EDIT”) and introduces OpenRPC event result schema inconsistencies (string schema while examples return null, unlike other events), which should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (2) — in code that hasn't changed since the last review.

src/json_types/videooutput.h:165

• This file is marked as auto-generated ("DO NOT EDIT"), but new enum adapter classes are added here. Please implement these adapters in the generator/templates and regenerate the module; otherwise the changes risk being lost on the next sync/regeneration.
  src/videooutput_impl.cpp:52
• This file is marked as auto-generated ("DO NOT EDIT"), but the PR changes the getter/subscription marshalling here. To avoid future regenerations overwriting the fix, update the generator/templates and re-generate the VideoOutput module instead of patching this file directly.

docs/openrpc/the-spec/firebolt-open-rpc.json:4161

• The OpenRPC method entry is missing the "property:readonly" tag used by other getter-style methods (e.g., Device.dolbyAtmosExperienceAvailable at docs/openrpc/the-spec/firebolt-open-rpc.json:857-866). Add the property tag to keep the fixture consistent.

			"tags": [
				{
					"name": "capabilities",
					"x-uses": [
						"xrn:firebolt:capability:cpp-client-only"
					]
				}
			],

docs/openrpc/the-spec/firebolt-open-rpc.json:4195

• The OpenRPC method entry is missing the "property:readonly" tag used by other getter-style methods (e.g., Device.dolbyAtmosExperienceAvailable at docs/openrpc/the-spec/firebolt-open-rpc.json:857-866). Add the property tag to keep the fixture consistent.

			"tags": [
				{
					"name": "capabilities",
					"x-uses": [
						"xrn:firebolt:capability:cpp-client-only"
					]
				}
			],

test/component/videooutputGeneratedTest.cpp:195

• These runtime subscription tests conditionally skip if the subscription call fails, which can mask regressions (the fixture in this repo now defines these events). Prefer failing the test via verifyEventSubscription(id) rather than skipping so CI catches missing/incorrect fixture wiring.

    if (!id)
    {
        GTEST_SKIP() << "VideoOutput.onRefreshRateChanged is not available in the current mock OpenRPC fixture";
    }
    verifyEventSubscription(id);

docs/openrpc/the-spec/firebolt-open-rpc.json:4319

• These event subscription methods use a non-null result schema (string enum) while their examples return null. Other event methods in this fixture consistently declare a null result schema (e.g., Localization.onCountryChanged at docs/openrpc/the-spec/firebolt-open-rpc.json:4436-4440), which matches the actual JSON-RPC subscribe response. Consider changing the result schema here to type "null" for consistency and to avoid schema/example mismatch.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

docs/openrpc/the-spec/firebolt-open-rpc.json:4370

• These event subscription methods use a non-null result schema (string enum) while their examples return null. Other event methods in this fixture consistently declare a null result schema (e.g., Localization.onCountryChanged at docs/openrpc/the-spec/firebolt-open-rpc.json:4436-4440), which matches the actual JSON-RPC subscribe response. Consider changing the result schema here to type "null" for consistency and to avoid schema/example mismatch.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The OpenRPC entries for VideoOutput.on*Changed methods declare a non-null result schema despite examples (and other events) using null, and the component tests currently skip on subscription failure which can mask regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

docs/openrpc/the-spec/firebolt-open-rpc.json:4267

• The event method example returns null, but the declared result schema is a string enum. This makes the OpenRPC entry internally inconsistent (and differs from other on*Changed entries which use a null result schema for the subscribe call). The payload schema should be derived via x-subscriber-for (i.e., VideoOutput.hdcp).

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

docs/openrpc/the-spec/firebolt-open-rpc.json:4319

• The event method example returns null, but the declared result schema is a string enum. This makes the OpenRPC entry internally inconsistent (and differs from other on*Changed entries which use a null result schema for the subscribe call). The payload schema should be derived via x-subscriber-for (i.e., VideoOutput.cecState).

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

docs/openrpc/the-spec/firebolt-open-rpc.json:4370

• The event method example returns null, but the declared result schema is a string enum. This makes the OpenRPC entry internally inconsistent (and differs from other on*Changed entries which use a null result schema for the subscribe call). The payload schema should be derived via x-subscriber-for (i.e., VideoOutput.refreshRate).

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

test/component/videooutputGeneratedTest.cpp:166

• This GTEST_SKIP() turns any subscription failure (schema mismatch, transport failure, etc.) into a skipped test, which can mask real regressions. Since verifyEventSubscription(id) already fails the test with the error, the skip block should be removed.

    if (!id)
    {
        GTEST_SKIP() << "VideoOutput.onCecStateChanged is not available in the current mock OpenRPC fixture";
    }
    verifyEventSubscription(id);

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The OpenRPC fixture updates are internally inconsistent (missing property:readonly tags) and appear incomplete relative to the PR description, so the spec/test metadata should be corrected before approval.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

docs/openrpc/the-spec/firebolt-open-rpc.json:4120

• PR description mentions adding missing VideoOutput getter/event entries to the OpenRPC fixture, but this change only adds hdcp, cecState, refreshRate and their events; other public VideoOutput APIs implemented in src/videooutput_impl.cpp (e.g., resolution/onResolutionChanged, colorDepth, colorFormat, colorimetry, dynamicRange, quantizationRange) are still absent from the fixture. Either the description should be narrowed, or the remaining methods/events should be added so the fixture fully represents the module API.

docs/openrpc/the-spec/firebolt-open-rpc.json:4161

• In the OpenRPC fixture, VideoOutput.cecState is a getter-style API but its tags block is missing the property:readonly tag (unlike VideoOutput.hdcp just above, and many other getters such as Localization.country). This can make tooling/tests treat it inconsistently as a non-property RPC.

			"tags": [
				{
					"name": "capabilities",
					"x-uses": [
						"xrn:firebolt:capability:cpp-client-only"

docs/openrpc/the-spec/firebolt-open-rpc.json:4195

• In the OpenRPC fixture, VideoOutput.refreshRate is a getter-style API but its tags block is missing the property:readonly tag (unlike VideoOutput.hdcp). This creates inconsistent metadata for the same module’s getter methods.

			"tags": [
				{
					"name": "capabilities",
					"x-uses": [
						"xrn:firebolt:capability:cpp-client-only"

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

It directly modifies files marked “AUTO-GENERATED — DO NOT EDIT” and introduces OpenRPC inconsistencies (event result schema vs examples, missing readonly tags) that should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

docs/openrpc/the-spec/firebolt-open-rpc.json:4161

• This getter is tagged as a property, but unlike other readonly getters in the OpenRPC fixture it is missing the "property:readonly" tag (e.g., Localization.country). Adding it keeps the spec metadata consistent for capability/property discovery.

			"tags": [
				{
					"name": "capabilities",
					"x-uses": [
						"xrn:firebolt:capability:cpp-client-only"

docs/openrpc/the-spec/firebolt-open-rpc.json:4195

• This getter is tagged as a property, but unlike other readonly getters in the OpenRPC fixture it is missing the "property:readonly" tag (e.g., Localization.country). Adding it keeps the spec metadata consistent for capability/property discovery.

			"tags": [
				{
					"name": "capabilities",
					"x-uses": [
						"xrn:firebolt:capability:cpp-client-only"

docs/openrpc/the-spec/firebolt-open-rpc.json:4316

• The examples for this event show a null JSON-RPC response (subscription acknowledgement), but the declared result schema is a string enum. This makes the OpenRPC entry internally inconsistent and unlike other event methods (e.g., Device.onHdrChanged) which declare a null result schema.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

docs/openrpc/the-spec/firebolt-open-rpc.json:4367

• The examples for this event show a null JSON-RPC response (subscription acknowledgement), but the declared result schema is a string enum. This makes the OpenRPC entry internally inconsistent and unlike other event methods (e.g., Device.onHdrChanged) which declare a null result schema.

			"result": {
				"name": "result",
				"schema": {
					"type": "string",
					"enum": [

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It modifies files marked “AUTO-GENERATED … DO NOT EDIT”, so the fix needs to be implemented in the generator and regenerated to avoid future overwrites and divergence.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

docs/openrpc/the-spec/firebolt-open-rpc.json:4277

• The PR description says VideoOutput event result schemas were corrected to string-enum schemas, and this file updates onCecStateChanged/onRefreshRateChanged accordingly, but VideoOutput.onHdcpChanged still declares a null result schema. This leaves the VideoOutput event definitions inconsistent; update onHdcpChanged to a string enum matching VideoOutput.hdcp for parity.

src/videooutput_impl.cpp:51

• src/videooutput_impl.cpp is marked "AUTO-GENERATED … DO NOT EDIT" (see file header). Editing generated output in-repo risks the next regeneration overwriting these fixes and makes it hard to keep the generator and SDK in sync. This change should be applied in firebolt-sdk-gen and the file re-generated.

Result<HdcpState> VideoOutputImpl::hdcp() const
{
    return helper_.get<JsonData::HdcpStateJson, HdcpState>("VideoOutput.hdcp");
}
Result<SubscriptionId> VideoOutputImpl::subscribeOnHdcpChanged(std::function<void(const HdcpState&)>&& notification)
{
    return subscriptionManager_.subscribe<JsonData::HdcpStateJson>("VideoOutput.onHdcpChanged", std::move(notification));

src/json_types/videooutput.h:162

• src/json_types/videooutput.h is marked "AUTO-GENERATED … DO NOT EDIT" (see file header). The new *Json adapter classes should be generated from the spec/generator rather than hand-edited here, otherwise the next regen will likely drop them and reintroduce the bug.

class CecStateValueJson : public Firebolt::JSON::NL_Json_Basic<::Firebolt::VideoOutput::CecStateValue>
{
public:
    void fromJson(const nlohmann::json& json) override
    {

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite