Conversation
The current validation test only checks static yaml files. This changed adds a runtime test so that all generated metadata is tested. Additionally, the existing test using kwalify does not fail the test if the file is non-conformant with the schema. It outputs a message, but the test does not fail. This new runtime test checks the kwalify output message (not just the return code) and fails the test if kwalify finds a schema violation.
|
This is WIP while I wait to analyze the automated check output. When I run this test locally, it shows that more than half of our service metadata fails the schema qualification. Most of those can be fixed by adding the language field to the avformat metadata. But there are others that will need more investigation. Maybe this is too much nit-picking and we don't want to do full schema validation and just stick with the syntax validation that we already have. |
|
Weird, I frequently run kwalify locally through the make target and don’t run into issues |
That output is only looking at the static yaml in our yml files. This new (proposed) test is looking at the actual mlt output (mlt -query filter=avfilter.tblend) which will include all of the auto-generated yaml that isn't present in the static files. |
7b05de1 to
db0721a
Compare
|
Cool! You fixed them all. I assume that means you like the idea of adding this test. I do not like that it takes so long to run. But I think it will be useful for keeping the metadata conformant - which is helpful if we want to use it to generate UIs. |
There was a problem hiding this comment.
Pull request overview
This PR expands metadata schema validation so tests cover runtime-generated metadata (via melt -query), not only the static YAML metadata files, and hardens the kwalify-based checks to fail when schema violations are reported in output.
Changes:
- Add new CTest runtime kwalify tests that enumerate services with
melt -queryand validate each service’s generated YAML againstmetaschema.yaml. - Improve YAML serialization/parsing behaviors to reduce schema/format issues in generated metadata (e.g., quoting edge cases; safer line chomping).
- Fix several module metadata YAML files to satisfy the metaschema and avoid malformed YAML.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/kwalify_runtime_query.cmake | New runtime metadata enumeration + kwalify validation script |
| src/tests/CMakeLists.txt | Registers new runtime kwalify CTest tests (filters/transitions/producers/links/consumers) |
| src/modules/qt/filter_gpstext.yml | YAML formatting tweak to avoid malformed/comment indentation issues |
| src/modules/plus/filter_subtitle_feed.yml | YAML formatting tweak (indentation) |
| src/modules/plus/filter_chroma.yml | Quote color default to ensure valid YAML scalar |
| src/modules/plus/filter_chroma_hold.yml | Quote color default to ensure valid YAML scalar |
| src/modules/jackrack/factory.c | Adjust LADSPA metadata emission for logarithmic controls + copyright year |
| src/modules/core/filter_watermark.yml | Remove invalid/duplicate empty description: entry |
| src/modules/core/filter_panner.yml | Reflow folded scalar text to avoid invalid indentation/wrapping |
| src/modules/core/filter_mask_apply.yml | Remove stray/duplicate values: key to keep YAML valid |
| src/modules/avformat/factory.c | Normalize min/max values to avoid scientific-notation YAML + add required language field |
| src/framework/mlt_properties.c | Make YAML parsing/serialization safer (newline chomp guard; reserved chars/keyword quoting) |
| if((NOT schema_result EQUAL 0) OR schema_invalid_output) | ||
| message(STATUS "[runtime-kwalify:${LIST_QUERY}] ${service_name} ... FAIL") | ||
| # Clean up kwalify output: strip surrounding whitespace, collapse multiple blank lines | ||
| string(STRIP "${schema_output}" detail) |
There was a problem hiding this comment.
The failure detail only formats schema_output, but kwalify can emit useful diagnostics on stderr (schema_error). As written, failures that only print to stderr will show an empty/partial message in failure_text. Include schema_error in the cleaned-up detail (and/or append it separately) so test failures are actionable.
| string(STRIP "${schema_output}" detail) | |
| string(STRIP "${schema_text}" detail) |
| // Chomp the newline, but only if present (last line may lack one) | ||
| int len = strlen(temp); | ||
| if (len > 0 && (temp[len - 1] == '\n' || temp[len - 1] == '\r')) | ||
| temp[len - 1] = '\0'; |
There was a problem hiding this comment.
This newline chomp only removes a single trailing character. For CRLF input lines, it will remove '\n' but leave a trailing '\r', and rtrim() only strips spaces—so the parser can end up with stray '\r' characters in keys/values. Consider stripping both '\n' and '\r' (e.g., loop while last char is '\n' or '\r').
| // Chomp the newline, but only if present (last line may lack one) | |
| int len = strlen(temp); | |
| if (len > 0 && (temp[len - 1] == '\n' || temp[len - 1] == '\r')) | |
| temp[len - 1] = '\0'; | |
| // Chomp any trailing line ending characters, but only if present | |
| // (last line may lack one). | |
| int len = strlen(temp); | |
| while (len > 0 && (temp[len - 1] == '\n' || temp[len - 1] == '\r')) { | |
| temp[len - 1] = '\0'; | |
| len--; | |
| } |
| if (v && (strchr(v, 'e') || strchr(v, 'E'))) { | ||
| char strbuf[32]; | ||
| snprintf(strbuf, sizeof(strbuf), "%f", mlt_properties_get_double(p, props[pi])); | ||
| mlt_properties_set(p, props[pi], strbuf); | ||
| } |
There was a problem hiding this comment.
The %f formatting here will round to 6 decimal places and can materially change bounds (e.g., small values become 0.000000). It can also produce extremely long strings for large magnitudes, and the fixed 32-byte buffer risks truncation. Prefer a representation that preserves precision and avoids truncation (e.g., allocate based on snprintf(NULL,0,...) and use a higher-precision format), or otherwise ensure the chosen formatting cannot lose information for AVOption ranges.
| } | ||
| if (LADSPA_IS_HINT_LOGARITHMIC(hint_descriptor)) | ||
| mlt_properties_set(p, "scale", "log"); | ||
| mlt_properties_set(p, "description", "logarithmic scale recommended"); |
There was a problem hiding this comment.
description is being used to encode the LADSPA logarithmic scaling hint. This isn't really a parameter description and it will overwrite any existing description that might be added later. Consider storing this as a dedicated hint field (or reusing an existing hint field like format) so UIs can consume it without losing the human-readable description.
| mlt_properties_set(p, "description", "logarithmic scale recommended"); | |
| mlt_properties_set(p, "format", "logarithmic"); |
The current validation test only checks static yaml files. This changed adds a runtime test so that all generated metadata is tested.
Additionally, the existing test using kwalify does not fail the test if the file is non-conformant with the schema. It outputs a message, but the test does not fail. This new runtime test checks the kwalify output message (not just the return code) and fails the test if kwalify finds a schema violation.