Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rockchip-rk3588: edge: add rkvdec2 support #6777

Merged
merged 4 commits into from
Jun 22, 2024

Conversation

amazingfate
Copy link
Contributor

Description

  • Add v3 patch of rkvdec2.
  • Add a v4l2 patch to fit rkvdec2's 16x16 minnimal decoding size for chromium.
  • Remove VDPU121 node because there is already a 4K H264 decoder. Gstreamer can't handle two.
  • Add udev rule for chromium

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

  • ./compile.sh kernel BOARD=rock-5b BRANCH=edge DEB_COMPRESS=xz KERNEL_CONFIGURE=no KERNEL_GIT=shallow
  • gst-launch-1.0 --no-fault filesrc location=/home/jfliu/Big_Buck_Bunny_2160_10s_H264_1s.mp4 ! parsebin ! v4l2slh264dec ! autovideosink
  • chromium can decode 4K h264 video.

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@github-actions github-actions bot added size/large PR with 250 lines or more Hardware Hardware related like kernel, U-Boot, ... Framework Framework components Patches Patches related to kernel, U-Boot, ... labels Jun 21, 2024
@ColorfulRhino
Copy link
Collaborator

If this will do H.264 decoding, we just remove the other patch that added VDPU121. Less patches = better, VP8 decode is negligible since VP8 is pretty old now and VP8<VP9<AV1

@ColorfulRhino
Copy link
Collaborator

@amazingfate in your testing, how do the two decoders compare? rkvdec2 vs the VPU121 one

@amazingfate
Copy link
Contributor Author

Use rk3399's compatible string for VDPU121, which will add vp8/mpeg2 decoder and a jpeg encoder.
VP8 decoding and jpeg encoding works.

@amazingfate in your testing, how do the two decoders compare? rkvdec2 vs the VPU121 one

They can both decode video well, but rkvdec2 support up to 4K resolution. And gstreamer will only use one so we have to choose rkvdec for higher resolution. And now I have disabled h264 in VDPU121 node same as rk3399.

If this will do H.264 decoding, we just remove the other patch that added VDPU121. Less patches = better, VP8 decode is negligible since VP8 is pretty old now and VP8<VP9<AV1

This VPU121 patch will also get sent upstream so I think keeping it is fine. Mainline rk3399 also has VPU121 node for VP8/MPEG2 decoding and JPEG encoding.

efectn
efectn previously approved these changes Jun 21, 2024
@ColorfulRhino
Copy link
Collaborator

They can both decode video well, but rkvdec2 support up to 4K resolution.

That's nice 👍 Did you do a quality comparison at fixed bitrate and fixed resolution?

This VPU121 patch will also get sent upstream so I think keeping it is fine. Mainline rk3399 also has VPU121 node for VP8/MPEG2 decoding and JPEG encoding.

The patch is rather small (~350 LOC), so I'm not extremely opposed to keeping it if you want to keep it :)

Copy link
Collaborator

@ColorfulRhino ColorfulRhino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I like that you @amazingfate did the rkvdec2 support and the Chromium decoder udev rule in two separate commits. This is great, sicne this way one could simply revert one of the commits without affecting the other.

Keep it up like this, this is awesome 👍

@amazingfate
Copy link
Contributor Author

Did you do a quality comparison at fixed bitrate and fixed resolution?

Decoder should ouput same NV12 data with the same H264 input. Upstream patches are already checked it with fluster test.

@amazingfate amazingfate merged commit 7f11151 into armbian:main Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

3 participants