decoder: h264: Use Cow instead of slice by bgrzesik · Pull Request #66 · chromeos/cros-codecs

bgrzesik · 2024-03-05T15:32:14Z

Fixes #44
Partially address #49
Relies on chromeos/cros-libva#10

bgrzesik · 2024-03-06T09:12:34Z

I tested the changes using fluster and there seems to be no effect on the test results

Gnurou

Looking good, a few comments inline. Having CL descriptions shortly explain what happens in the CL would also make reviews easier imho.

Gnurou · 2024-03-08T03:34:36Z

src/codec/h264/nalu.rs

-            offset: nalu_offset,
-            sc_offset: start_code_offset,
+            offset: nalu_offset - start_code_offset,
+            sc_offset: 0,


This looks unrelated to the switch to Cow. Actually, this looks more like it should be part of the next commit that removes sc_offset?

Yeah, It it was related to the comment below. But I've moved to the next commit.

Gnurou · 2024-03-08T03:37:56Z

src/utils.rs

-                let end = n.offset + n.size;
-                &n.data[start..end]
-            })
-            .ok()


Same here, this should be unrelated to using Cow IIUC.

Unfortunately it is

error[E0515]: cannot return value referencing local data `n.data` --> src/utils.rs:175:17 | 175 | &n.data[start..end] | ^------^^^^^^^^^^^^ | || | |`n.data` is borrowed here | returns a value referencing data owned by the current function

I've moved slicing the bitstream to a separate commit, however before that commit this slicing had to be handled separately for Cow::Owned , like it is now.

Gnurou · 2024-03-08T03:38:04Z

src/utils.rs

-                let start = n.sc_offset;
-                let end = n.offset + n.size;
-                &n.data[start..end]
-            })


src/codec/h264/nalu.rs

src/decoder/stateless/h265.rs

bgrzesik

Thank you @Gnurou for the review! Hopefully it's better now 😁

bgrzesik · 2024-03-08T14:32:36Z

src/utils.rs

-                let end = n.offset + n.size;
-                &n.data[start..end]
-            })
-            .ok()


Unfortunately it is

error[E0515]: cannot return value referencing local data `n.data` --> src/utils.rs:175:17 | 175 | &n.data[start..end] | ^------^^^^^^^^^^^^ | || | |`n.data` is borrowed here | returns a value referencing data owned by the current function

I've moved slicing the bitstream to a separate commit, however before that commit this slicing had to be handled separately for Cow::Owned , like it is now.

bgrzesik · 2024-03-08T14:32:50Z

src/codec/h264/nalu.rs

-            offset: nalu_offset,
-            sc_offset: start_code_offset,
+            offset: nalu_offset - start_code_offset,
+            sc_offset: 0,


Yeah, It it was related to the comment below. But I've moved to the next commit.

src/decoder/stateless/h265.rs

src/codec/h264/nalu.rs

Gnurou · 2024-03-13T05:29:42Z

So unfortunately I am getting test failures on AMD when decoder: h264: vaapi: Use slice arrays instead multiple buffers is applied...

---- decoder::stateless::h264::vaapi::tests::test_25fps_nonblock stdout ----
thread 'decoder::stateless::h264::vaapi::tests::test_25fps_nonblock' panicked at src/decoder/stateless.rs:399:21:
assertion `left == right` failed: at frame 0
  left: "f3fd3133"
 right: "3eae0fa4"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- decoder::stateless::h264::vaapi::tests::test_25fps_block stdout ----
thread 'decoder::stateless::h264::vaapi::tests::test_25fps_block' panicked at src/decoder/stateless.rs:399:21:
assertion `left == right` failed: at frame 0
  left: "f3fd3133"
 right: "3eae0fa4"

Possibly AMD doesn't like slice arrays? That would be quite an issue...

Gnurou · 2024-03-13T07:17:59Z

Pushed the first 3 commits (and an update to cros-libva so build passes on the PR) - let's see if we can figure out the AMD issue...

This changes is necessary for the next change, that will require picture to contain copies of slices.

With this commit the chromeos#44 is fixed. It is achieved by submitting multiple slices of one picture using a single slice data buffer and all slice parameters using a single buffer with multiple elements.

bgrzesik · 2024-03-13T13:33:41Z

Possibly AMD doesn't like slice arrays? That would be quite an issue...

Yeah, I can confirm the AMD does not like slice arrays (source). Assert fails on debug mesa.

Gnurou · 2024-03-22T07:03:37Z

I'm wondering the reason for this limitation - tried to look at related issues on Gitlab, but could not find a matching one. Maybe we should open a bug?

bgrzesik · 2024-03-22T10:52:33Z

I'm wondering the reason for this limitation - tried to look at related issues on Gitlab, but could not find a matching one.

When I briefly looked at the code it seemed if I understand it correctly, the slice data is submitted to the hw on vaRenderBuffer, so I think it's just a design choice.

I wonder how other users approaches this issue and if it is an Intel driver issue. I tried running ToT ccdec with video generated using gst-launch-1.0 videotestsrc num-buffers=1000 ! 'video/x-raw,width=320,height=240,format=NV12' ! x264enc option-string="slices=100" ! filesink location=a.h264 and checked the md5 with ffmpeg and checksums were the same.

Maybe we should open a bug?

Nevertheless I think it could be worth it

bgrzesik marked this pull request as ready for review March 6, 2024 09:12

Gnurou approved these changes Mar 8, 2024

View reviewed changes

Gnurou assigned bgrzesik and Gnurou and unassigned bgrzesik Mar 8, 2024

bgrzesik force-pushed the dec-h26x-cow branch 2 times, most recently from 39c9a51 to 3039f54 Compare March 8, 2024 14:50

bgrzesik commented Mar 8, 2024

View reviewed changes

bgrzesik added 2 commits March 13, 2024 09:49

decoder: h264: vaapi: Use codec specific picture

212e96d

This changes is necessary for the next change, that will require picture to contain copies of slices.

decoder: h264: vaapi: Use slice arrays instead multiple buffers

52e4af7

With this commit the chromeos#44 is fixed. It is achieved by submitting multiple slices of one picture using a single slice data buffer and all slice parameters using a single buffer with multiple elements.

bgrzesik force-pushed the dec-h26x-cow branch from 3039f54 to 52e4af7 Compare March 13, 2024 08:51

Gnurou force-pushed the main branch from ae34408 to 234f7b1 Compare June 26, 2024 03:54

Conversation

bgrzesik commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgrzesik commented Mar 6, 2024

Uh oh!

Gnurou left a comment

Choose a reason for hiding this comment

Uh oh!

Gnurou Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

bgrzesik Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Gnurou Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

bgrzesik Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Gnurou Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bgrzesik left a comment

Choose a reason for hiding this comment

Uh oh!

bgrzesik Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

bgrzesik Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Gnurou commented Mar 13, 2024

Uh oh!

Gnurou commented Mar 13, 2024

Uh oh!

bgrzesik commented Mar 13, 2024

Uh oh!

Gnurou commented Mar 22, 2024

Uh oh!

bgrzesik commented Mar 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bgrzesik commented Mar 5, 2024 •

edited

Loading