Support for Shikra added for Video - #569
Conversation
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
Recommended scalable design
functestlib.sh
platform_runtime_identity()
platform_identity_matches()
lib_video.sh
video_policy_lookup(platform, mode, codec)
video_stage_control_override(config, control, value, output_dir)
run.sh
platform=$(...)
policy=$(...)
effective_config=$(...)
run repeats and retries with effective_config
With this design:
- Adding a new SoC means adding policy data.
- Platform detection remains shared across audio and video.
- JSON staging has one implementation.
- Normal runs and retries cannot diverge.
- No-policy platforms use the original config unchanged.
| # if no device-tree/platform data is available (callers should treat that | ||
| # as "no match" rather than falling back to a caller-name heuristic). | ||
| # ----------------------------------------------------------------------------- | ||
| video_get_dt_identity() { |
There was a problem hiding this comment.
video_get_dt_identity() duplicates both detect_platform() and the Shikra runtime identity logic merged in PR #544 as audio_platform_is_shikra(). A future SoC alias or DT-root correction would need to be updated independently in functestlib, audio, and video. Extract a generic runtime platform identity/match helper into functestlib.sh, including both DT roots, and make audio and video consume that shared contract.
| # video_target_is_shikra -> 0/1 | ||
| # video_apply_level_override_for_target <cfg_json_path> | ||
|
|
||
| : "${VIDEO_LEVEL_OVERRIDE:=4.0}" |
There was a problem hiding this comment.
The policy model supports only one global target/value pair:
VIDEO_LEVEL_OVERRIDE=4.0
VIDEO_LEVEL_OVERRIDE_TARGET=shikra
It cannot express cases such as Shikra H.264=4.0, Shikra HEVC=4.1, and a future SoC HEVC=5.1 without adding branches or changing global state. Replace this with a policy lookup keyed by platform, mode, codec, control, and value. Adding another SoC should require one policy row, with no run.sh changes.
For example:
shikra|encode|h264|Level|4.0
shikra|encode|hevc|Level|4.0
new-soc|encode|hevc|Level|5.1
| } | ||
|
|
||
| # Convenience wrapper for the "shikra" target specifically. | ||
| video_target_is_shikra() { |
There was a problem hiding this comment.
video_target_is_shikra() has no callers and establishes a pattern that would lead to video_target_is_() helpers for every new target. Remove this wrapper. Use the shared platform identity plus the generic policy lookup.
| if video_run_once "$cfg" "$logf" "$TIMEOUT" "$SUCCESS_RE" "$LOGLEVEL"; then | ||
| run_cfg="$cfg" | ||
| if [ "$mode" = "encode" ] && { [ "$codec" = "h264" ] || [ "$codec" = "hevc" ]; }; then | ||
| if command -v video_apply_level_override_for_target >/dev/null 2>&1; then |
There was a problem hiding this comment.
Configuration preparation is duplicated in the normal path and retry path at line 1262. Every future target policy would need to remain synchronized across both branches. Resolve and stage the effective configuration once per test case, before the repeat loop, then use the same path for normal
runs and retries. Preserve helper diagnostics instead of redirecting stderr.
| BEGIN { in_level = 0 } | ||
| { | ||
| line = $0 | ||
| if (line ~ /"Id"[ \t]*:[ \t]*"Level"/) { |
There was a problem hiding this comment.
The transformation claims to be scoped to StaticControls, but it only finds "Id": "Level" and rewrites the next "Value" line. It does not track the enclosing JSON object. Make the generic staging helper structurally validate the intended object and verify that exactly one requested control changed. A malformed or ambiguous config must fail preparation rather than run with an uncertain result.
| *"$tok_l"*) | ||
| return 0 | ||
| ;; | ||
| *) |
There was a problem hiding this comment.
Remove the trailing whitespace. git diff --check currently fails here.
bbbb19e to
38ba8f4
Compare
|
Hi Srikanth Muppandam (@smuppand) |
|
|
||
| if printf '%s' "$s" | grep -Eq "$shikra_pat"; then | ||
| printf '%s\n' "shikra" | ||
| return 0 |
There was a problem hiding this comment.
Returning shikra here introduces a platform value that none of the stack helpers recognize.
With the default VIDEO_STACK=auto, stack detection returns unknown, selects upstream, and the runner exits at the hard stack gate before applying the Shikra configuration policy.
Please define Shikra’s stack contract across video_stack_status, validation, blacklist, and switching paths, or explicitly bypass legacy stack switching with an appropriate runtime readiness check.
| return 0 | ||
| } | ||
|
|
||
| # ----------------------------------------------------------------------------- |
There was a problem hiding this comment.
WK status 1 conflates an already-correct value with a target control whose object has no valid Value. The latter is documented as malformed but is returned as success, allowing Shikra to run the original unsupported level.
Please distinguish “Value found and already correct” from “target Id found without a valid Value”, and fail configuration preparation for the malformed case.
397925a to
8a7da66
Compare
8a7da66 to
c9ae0a4
Compare
|
Hi Srikanth Muppandam (@smuppand) Improved Malformed control configurations now fail preparation instead of being silently ignored, preventing test execution with unsupported settings. |
| [ -z "$vsc_val" ] && return 1 | ||
|
|
||
| # Quick check: does this file have the target control at all? | ||
| if ! grep -q "\"Id\"[[:space:]]*:[[:space:]]*\"${vsc_ctrl}\"" "$vsc_cfg" 2>/dev/null; then |
There was a problem hiding this comment.
The Shikra policy declares Level=4.0 as required, but a config without that control returns success here. The runner then executes the original config, silently bypassing the platform policy and potentially running an unsupported driver default.
Return a distinct “required control absent” error when a matching policy row is applied, and fail config preparation rather than treating it as a no-op.
| END { | ||
| # Id found but no Value found in the object — malformed config. | ||
| if (found_id > 0 && found_val == 0) exit 2 | ||
| # Value found but already at the desired setting — valid no-op. |
There was a problem hiding this comment.
found_id is incremented but never used to enforce uniqueness. Two Level objects with already-correct values return the valid-no-op status, and a mixed pair can produce one rewrite and still succeed. That contradicts the helper contract that exactly one control object must match.
Fail when found_id != 1 or found_val != 1, independently of the number of value changes.
| # platform vars (PLATFORM_MACHINE, PLATFORM_TARGET, PLATFORM_SOC_MACHINE, | ||
| # PLATFORM_DT_MODEL, PLATFORM_DT_COMPAT). | ||
| # | ||
| # This is the SINGLE shared source of truth for "what board am I running on", |
There was a problem hiding this comment.
This says the helper is the shared source of truth “consumed by both audio and video,” but audio_common.sh still has its own audio_platform_is_shikra() DT scan.
Rework audio to consume platform_identity_matches shikra, then remove the duplicate scan. Otherwise future DT-root or identity changes will still diverge across audio and video.
| # device-tree/platform data is available. Callers should treat an empty | ||
| # result as "no match" rather than falling back to a hostname heuristic. | ||
| # ----------------------------------------------------------------------------- | ||
| platform_runtime_identity() { |
There was a problem hiding this comment.
detect_platform() already provides platform DT fields. The new shared identity helper is a reasonable extension, but its intended reuse is incomplete because audio still maintains a parallel implementation.
3 participants
Changes: