Repository navigation
Convert video to RGB on the CPU with the matrix of the stream - #6569
Open
jantonguirao wants to merge 4 commits into
Open
jantonguirao wants to merge 4 commits into
jantonguirao wants to merge 4 commits into
Conversation
The CPU video decoder always used the BT.601 matrix for the source in swscale, while HD video is normally BT.709, which gives visibly wrong colors for saturated colors (e.g. pure red 255,0,0 came out as ~234,0,3). Use the BT.709 matrix when the stream is tagged as such; streams that don't specify it keep using BT.601. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
Contributor
 [Medium impact] The PR appears safe to merge; no actionable issues remain.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The BT.709 source-table selection also affects YCbCr output, causing libswscale to rematrix it to BT.601 and unintentionally change previously unaffected results.
1 open issue
What changed in this PR
Updates the shared CPU video decoder to use BT.709 metadata when converting video to RGB, correcting colors in tagged streams while retaining BT.601 as the fallback.
Changes:
- Selects the source matrix from frame metadata, falling back to stream metadata.
- Reconfigures swscale when the source matrix or range changes.
- Adds a VP9 regression test against ffmpeg references from the linked DALI_extra fixtures.
| File | Description |
|---|---|
dali/test/python/decoder/test_video.py |
Adds color-bar comparisons and a BT.709 CPU decoding regression test. |
dali/operators/video/frames_decoder_cpu.h |
Tracks the source matrix configured in swscale. |
dali/operators/video/frames_decoder_cpu.cc |
Selects conversion coefficients from color metadata and updates swscale configuration. |
Review effort: Balanced
| AVColorSpace src_colorspace = frame_->colorspace != AVCOL_SPC_UNSPECIFIED | ||
| ? frame_->colorspace | ||
| : codec_params_->color_space; | ||
| int sws_colorspace = src_colorspace == AVCOL_SPC_BT709 ? SWS_CS_ITU709 : SWS_CS_DEFAULT; |
YCbCr output is a copy of the decoded planes, so the source matrix must stay the one of the destination. Also, run the CPU test on hosts without a GPU and build the paths with os.path.join. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
The test pattern in DALI_extra has 10 bars now (black and white were added): use their width in the helper that compares the bars, and check that black and white are mapped to the ends of the range (the luma is not left at 16 and 235). Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
The color matrix tests read db/video/color_matrix, which DALI_extra#137 adds. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
The CPU video decoder (
fn.experimental.readers.video,fn.experimental.decoders.videoandfn.experimental.inputs.videoon the CPU) converts YCbCr to RGB with swscale, and always tells it that the source uses the default matrix, ITU-R BT.601, whatever the stream says. HD video is normally BT.709, so its colors are wrong, noticeably for saturated colors: for example, pure red (255, 0, 0) in a BT.709 stream is decoded as about (234, 0, 3).This PR tells swscale that the source is BT.709 when the stream is tagged as such, and keeps BT.601 for every other stream (untagged streams are not affected). The matrix is read from the decoded frame, or from the stream parameters if the frame doesn't have it, as is already done for the range, and swscale is reconfigured when either changes.
It is based on
mainand independent of the GPU change (#6568, which does the same for the GPU decoder). The new test uses the video clips and the ffmpeg references added in NVIDIA/DALI_extra#137 (db/video/color_matrix), which is merged:DALI_EXTRA_VERSIONis updated to the commit that merged it.On the test clip (1080p VP9, tagged BT.709, limited range; the CPU decoder supports only VP8 and VP9) the error of the mean color of the saturated bars against the original pattern, and the mean difference of the whole frame against ffmpeg's decode (the reference), are:
(ffmpeg itself is within 2.0 of the original pattern.) Only BT.709 is added; other matrices are still converted as BT.601.
Additional information:
Affected modules and functionalities:
dali/operators/video/frames_decoder_cpu.{h,cc}: the source color space given tosws_setColorspaceDetails, and the member that remembers it.dali/test/python/decoder/test_video.py: new test.Key points relevant for the review:
image_type=YCbCr, where the source matrix is not used).CopyToOutputinframes_decoder_cpu.cc, next to code that other open PRs touch (e.g. Do not wait on the host for every picture the GPU video decoder converts #6561-Run the video reader's decode lanes on their own threads, a batch ahead #6564), so whichever merges second may need a trivial rebase. The new test adds a helper (color_matrix_bars_error) that the GPU PR (Convert video to RGB on the GPU with the BT.709 matrix when the stream uses it #6568) adds as well.Tests:
decoder/test_video.py: all of it (the GPU and CPU decoders,test_full_range_video*, the codec support tests).New test:
decoder/test_video.py::test_color_matrix_bt709_cpu, on the VP9 clip. It checks the mean color of the interior of each of the 10 bars of the pattern (the 8 colors, a black and a white bar) against ffmpeg's decode (at most 4 apart; BT.601 gives 22) and the mean difference of the whole frame (below 2). It also checks that the black bar is at most 2 and the white bar at least 253, i.e. that the luma is mapped to the whole range and clamped there (Y=16 and Y=235 in the limited range); those two bars do not depend on the matrix, so that check passes without this change too, and the saturated bars are the ones that fail without it.Run locally with this change on top of the other two:
test_video.pypasses (136 tests, 10 skipped). After the black and white bars were added to the clips, I ran test_color_matrix_bt709_cpu again with the new clips: the case pass.Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-4954