From cfb41c13585fe91d2dd7dfde88bad6150d368849 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9mence=20Lesn=C3=A9?= Date: Wed, 8 Apr 2026 18:58:00 +0200 Subject: [PATCH] fix: address CodeRabbit review findings - Add checkout step before local composite action (GitHub Actions requires repo on disk to read action.yml) - Use PR base SHA for path detection in multi-commit PRs - Fail benchmarks loudly on decode errors (.expect instead of let _ =) - Document intentional encode error handling (codec may drop frames) Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/actions/setup-linux/action.yml | 5 ----- .github/workflows/_bench.yml | 4 ++++ .github/workflows/ci.yml | 13 +++++++++++-- libs/scrap/benches/codec_decode.rs | 18 +++++++++--------- libs/scrap/benches/codec_encode.rs | 16 ++++++++++------ libs/scrap/benches/pipeline_decode.rs | 10 +++++----- 6 files changed, 39 insertions(+), 27 deletions(-) diff --git a/.github/actions/setup-linux/action.yml b/.github/actions/setup-linux/action.yml index 98d59df00..befbb8abb 100644 --- a/.github/actions/setup-linux/action.yml +++ b/.github/actions/setup-linux/action.yml @@ -30,11 +30,6 @@ runs: core.exportVariable('ACTIONS_CACHE_URL', process.env.ACTIONS_CACHE_URL || ''); core.exportVariable('ACTIONS_RUNTIME_TOKEN', process.env.ACTIONS_RUNTIME_TOKEN || ''); - - name: Checkout source code - uses: actions/checkout@v4 - with: - submodules: recursive - - name: Install prerequisites shell: bash run: | diff --git a/.github/workflows/_bench.yml b/.github/workflows/_bench.yml index 231aeb1d7..375f5d93c 100644 --- a/.github/workflows/_bench.yml +++ b/.github/workflows/_bench.yml @@ -16,6 +16,10 @@ jobs: name: Performance benchmarks runs-on: ubuntu-latest steps: + - uses: actions/checkout@v4 + with: + submodules: recursive + - uses: ./.github/actions/setup-linux - name: Install cargo-codspeed diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 67310ec1e..19119eeff 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,10 @@ jobs: job: - { target: x86_64-unknown-linux-gnu, os: ubuntu-24.04 } steps: + - uses: actions/checkout@v4 + with: + submodules: recursive + - uses: ./.github/actions/setup-linux - name: Show version information (Rust, cargo, GCC) @@ -75,10 +79,15 @@ jobs: steps: - uses: actions/checkout@v4 with: - fetch-depth: 2 + fetch-depth: 0 - id: check run: | - CHANGED=$(git diff --name-only HEAD~1 HEAD -- libs/scrap/ libs/hbb_common/ 2>/dev/null || echo "") + if [ "${{ github.event_name }}" = "pull_request" ]; then + BASE="${{ github.event.pull_request.base.sha }}" + else + BASE="HEAD~1" + fi + CHANGED=$(git diff --name-only "$BASE" HEAD -- libs/scrap/ libs/hbb_common/ 2>/dev/null || echo "") echo "run=$( [ -n "$CHANGED" ] && echo true || echo false )" >> "$GITHUB_OUTPUT" benchmarks: diff --git a/libs/scrap/benches/codec_decode.rs b/libs/scrap/benches/codec_decode.rs index 7e0155830..959bbe4fe 100644 --- a/libs/scrap/benches/codec_decode.rs +++ b/libs/scrap/benches/codec_decode.rs @@ -91,13 +91,13 @@ fn bench_decode_single(c: &mut Criterion) { let mut idx = 0; b.iter(|| { let union = &unions[idx % unions.len()]; - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( black_box(union), &mut rgb, &mut texture, &mut pixelbuffer, &mut chroma, - ); + ).expect("decode failed"); idx += 1; }); }); @@ -118,13 +118,13 @@ fn bench_decode_single(c: &mut Criterion) { let mut idx = 0; b.iter(|| { let union = &unions[idx % unions.len()]; - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( black_box(union), &mut rgb, &mut texture, &mut pixelbuffer, &mut chroma, - ); + ).expect("decode failed"); idx += 1; }); }); @@ -145,13 +145,13 @@ fn bench_decode_single(c: &mut Criterion) { let mut idx = 0; b.iter(|| { let union = &unions[idx % unions.len()]; - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( black_box(union), &mut rgb, &mut texture, &mut pixelbuffer, &mut chroma, - ); + ).expect("decode failed"); idx += 1; }); }); @@ -181,13 +181,13 @@ fn bench_vp9_decode_sequence(c: &mut Criterion) { group.bench_function(BenchmarkId::from_parameter("100frames_1080p"), |b| { b.iter(|| { for union in &unions { - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( black_box(union), &mut rgb, &mut texture, &mut pixelbuffer, &mut chroma, - ); + ).expect("decode failed"); } }); }); @@ -216,7 +216,7 @@ fn bench_vp9_decode_4k(c: &mut Criterion) { let mut idx = 0; b.iter(|| { let union = &unions[idx % unions.len()]; - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( black_box(union), &mut rgb, &mut texture, diff --git a/libs/scrap/benches/codec_encode.rs b/libs/scrap/benches/codec_encode.rs index e551c150e..a02bd2721 100644 --- a/libs/scrap/benches/codec_encode.rs +++ b/libs/scrap/benches/codec_encode.rs @@ -49,7 +49,8 @@ fn bench_vpx_encode_single(c: &mut Criterion) { let mut pts = 0i64; b.iter(|| { let input = EncodeInput::YUV(&yuv); - let _ = encoder.encode_to_message(input, pts); + // encode_to_message may return Err("no valid frame") when the codec drops a frame — this is normal +drop(encoder.encode_to_message(input, pts)); pts += 1; }); }); @@ -71,7 +72,8 @@ fn bench_vpx_encode_single(c: &mut Criterion) { let mut pts = 0i64; b.iter(|| { let input = EncodeInput::YUV(&yuv); - let _ = encoder.encode_to_message(input, pts); + // encode_to_message may return Err("no valid frame") when the codec drops a frame — this is normal +drop(encoder.encode_to_message(input, pts)); pts += 1; }); }); @@ -109,7 +111,8 @@ fn bench_encode_4k(c: &mut Criterion) { let mut pts = 0i64; b.iter(|| { let input = EncodeInput::YUV(black_box(&yuv)); - let _ = encoder.encode_to_message(input, pts); + // encode_to_message may return Err("no valid frame") when the codec drops a frame — this is normal +drop(encoder.encode_to_message(input, pts)); pts += 1; }); }); @@ -141,7 +144,7 @@ fn bench_vp9_encode_sequence_static(c: &mut Criterion) { b.iter(|| { for i in 0..100 { let input = EncodeInput::YUV(black_box(&yuv)); - let _ = encoder.encode_to_message(input, i); + drop(encoder.encode_to_message(input, i)); } }); }); @@ -174,7 +177,7 @@ fn bench_vp9_encode_sequence_movement(c: &mut Criterion) { b.iter(|| { for (i, yuv) in frames.iter().enumerate() { let input = EncodeInput::YUV(black_box(yuv)); - let _ = encoder.encode_to_message(input, i as i64); + drop(encoder.encode_to_message(input, i as i64)); } }); }); @@ -211,7 +214,8 @@ fn bench_vp9_encode_quality(c: &mut Criterion) { let mut pts = 0i64; b.iter(|| { let input = EncodeInput::YUV(black_box(&yuv)); - let _ = encoder.encode_to_message(input, pts); + // encode_to_message may return Err("no valid frame") when the codec drops a frame — this is normal +drop(encoder.encode_to_message(input, pts)); pts += 1; }); }); diff --git a/libs/scrap/benches/pipeline_decode.rs b/libs/scrap/benches/pipeline_decode.rs index 989390726..1f60e2678 100644 --- a/libs/scrap/benches/pipeline_decode.rs +++ b/libs/scrap/benches/pipeline_decode.rs @@ -83,13 +83,13 @@ fn bench_pipeline_decode_1080p(c: &mut Criterion) { // Step 1: Protobuf deserialize // Step 2+3: Decode + YUV→RGB via real Decoder::handle_video_frame if let Some(union) = extract_union(black_box(msg_bytes)) { - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( &union, &mut rgb, &mut texture, &mut pixelbuffer, &mut chroma, - ); + ).expect("decode failed"); } idx += 1; @@ -121,13 +121,13 @@ fn bench_pipeline_decode_4k(c: &mut Criterion) { b.iter(|| { let msg_bytes = &messages[idx % messages.len()]; if let Some(union) = extract_union(black_box(msg_bytes)) { - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( &union, &mut rgb, &mut texture, &mut pixelbuffer, &mut chroma, - ); + ).expect("decode failed"); } idx += 1; black_box(rgb.raw.len()) @@ -160,7 +160,7 @@ fn bench_pipeline_decode_sequence(c: &mut Criterion) { b.iter(|| { for msg_bytes in &messages { if let Some(union) = extract_union(msg_bytes) { - let _ = decoder.handle_video_frame( + decoder.handle_video_frame( &union, &mut rgb, &mut texture,