bcm2835-codec: Wrong colors on encoding BGR24 to H.264

mdevaev commented 7 months ago

Describe the bug I'm trying to encode a BGR24 stream in H.264 and get the wrong colors comparable to UYVY. A similar problem with UYVY -> JPEG. Is it possible to fix this?

UYVY -> H.264: good colors.
BGR24 -> H.264: the image is too highlighted (see the picture below).
UYVY -> JPEG (on /dev/video31): The image is too dark.
BGR24 -> JPEG: good colors.

To reproduce Encode the raw video from described format and see the result. Personally I'm using TC358743 capture for this.

Expected behaviour Correct colors.

System

CM4
Arch Linux ARM
version c14213886ae1e31951022b74ce776ce7ee7023ee (clean) (release) (start)
Linux pikvm 6.6.21-4-rpi #1 SMP Wed Mar 13 20:36:42 UTC 2024 armv7l GNU/Linux

Additional context

I need to use BGR24 specifically because it is the only format that supports DRM. I am trying to output an image to both DRM and H.264 stream at the same time.
My code: https://github.com/pikvm/ustreamer/blob/master/src/ustreamer/m2m.c#L223
https://forums.raspberrypi.com/viewtopic.php?p=2207220
An example for H.264 encoding:

6by9 commented 7 months ago

Tracing the colour conversion matrices, video encode appears to be converting from RGB to full range BT601 RGB, but then not inserting that information into the SPS header of the stream.

Limited range is more usual for video codecs, so most decoders will default to that in the absence of other information. Choosing BT601 / 709 / 2020 in that case is less well defined. Probably the best thing is to switch to BT601 limited range, and include the metadata so there is no ambiguity.

YUV is taken as-is, but I suspect again it has lost the range and colourspace metadata.

mdevaev commented 7 months ago

All this should be done inside the firmware, am I right?

6by9 commented 7 months ago

Yes it wants to be corrected in the firmware.

You may be able to tell your renderer to use full range regardless of what the stream says (or doesn't say). DRM has the "COLOR_RANGE" property to set full or limited range.

mdevaev commented 7 months ago

DRM works fine in my case. It just shows the captured BGR24. As for H.264, it is sent to the browser via WebRTC. But thanks for the advice, maybe I'll need it in the future.

If you take the time to look at this problem, I'm ready to test it, as usual. This is the only thing currently blocking us from releasing a big feature to customers.

6by9 commented 7 months ago

I was referring to using DRM to display the decoded frames. If you're going via webrtc than intercepting that is unlikely to be an option.

mdevaev commented 7 months ago

Yes, that's why I need it to be fixed in the encoder.

mdevaev commented 4 months ago

Do you have time to look at this in the foreseeable future? Our customers liked the feature, but the color discrepancy is frustrating.

6by9 commented 4 months ago

Sorry for the delay. Test firmware files attached that should give sensible colourspace information where provided. You'll need https://github.com/raspberrypi/linux/pull/6253 to work with it.

For YUV formats it should encode the colourspace information from userspace into the bitstream headers. For RGB formats, it should convert to BT601 limited range for SD resolutions, and BT709 limited range for anything greater than 720x576. That should also be encoded into the bitstream headers.

Please let me know if that solves your problem, and I'll get them merged.

fw_1885.zip

mdevaev commented 3 months ago

Thank you for your work. I've tested it but it causes massive failures. TTY log attached. ttylog.txt

6by9 commented 3 months ago

The change itself is only in configuration of the ISP, so can't inherently cause that sort of crash. What kernel version were you using before?

I'll try to find a few moments to set up a B102 and test in GStreamer. It was giving me the identical numbers when using videotestsrc for it generating I420, vs for it generating RGB, fed through v4l2h264enc (which converts to YUV), and decoded with libav.

mdevaev commented 3 months ago

I took bootloader 4a1d5d3ff87108ca46bf2d5132c8a1a936eeb4d2 and kernel https://github.com/raspberrypi/linux/commit/3b42260d2130b5ca110c5340ab2bd055eede5968, armv7l

Should I try the latest master?

6by9 commented 3 months ago

https://github.com/raspberrypi/linux/pull/6253 isn't merged yet, and you will need that or you may get weird things reported. Easiest is to use sudo rpi-update pulls/6253 to get the relevant kernel and latest kernel, and then overwrite the firmware files with those I attached above.

I'm using GStreamer as usual, and getting expected results. I'm using a Pi and kmstest --flip as the source. It's to a Pi4 with B101 (hence only allowing 1080p30) for capture.

v4l2-ctl --set-edid=file=1080P30EDID.txt
v4l2-ctl --set-dv-bt-timings query
gst-launch-1.0 -vvve v4l2src ! "video/x-raw,format=(string)RGB" ! capssetter caps="video/x-raw,format=RGB" ! v4l2h264enc ! "video/x-h264,level=(string)4" ! h264parse ! matroskamux ! filesink location=tc358743_rgb.mkv

The capssetter stuff is due colorimetry mismatches between V4L2 devices, so it just allows them to go with whatever their default is. It's fairly irrelevant for RGB anyway.

ffprobe reports the format as yuv420p(tv, bt709, progressive) for 1080p, and yuv420p(tv, smpte170m, progressive) for VGA, or 720x480p, which is as I expect.

mdevaev commented 3 months ago

I applied that patch manually on mykernel. Very weird. Okay, I'll try again from scratch, recheck all and report you back.

Чт, 11 июля 2024 г. в 15:02, 6by9 @.***>:

raspberrypi/linux#6253 https://github.com/raspberrypi/linux/pull/6253 isn't merged yet, and you will need that or you may get weird things reported. Easiest is to use sudo rpi-update pulls/6253 to get the relevant kernel and latest kernel, and then overwrite the firmware files with those I attached above.

I'm using GStreamer as usual, and getting expected results. I'm using a Pi and kmstest --flip as the source. It's to a Pi4 with B101 (hence only allowing 1080p30) for capture.

v4l2-ctl --set-edid=file=1080P30EDID.txt v4l2-ctl --set-dv-bt-timings query gst-launch-1.0 -vvve v4l2src ! "video/x-raw,format=(string)RGB" ! capssetter caps="video/x-raw,format=RGB" ! v4l2h264enc ! "video/x-h264,level=(string)4" ! h264parse ! matroskamux ! filesink location=tc358743_rgb.mkv

The capssetter stuff is due colorimetry mismatches between V4L2 devices, so it just allows them to go with whatever their default is. It's fairly irrelevant for RGB anyway.

ffprobe reports the format as yuv420p(tv, bt709, progressive) for 1080p, and yuv420p(tv, smpte170m, progressive) for VGA, or 720x480p, which is as I expect.

— Reply to this email directly, view it on GitHub https://github.com/raspberrypi/firmware/issues/1885#issuecomment-2222749801, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADFUA4IPAUKK5GKNMD6YWTZLZX45AVCNFSM6AAAAABFNGEBJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSG42DSOBQGE . You are receiving this because you authored the thread.Message ID: @.***>

mdevaev commented 3 months ago

Hello. I checked everything several times:

I compiled kernel from this commit https://github.com/raspberrypi/linux/commit/1abc413af44652d6a76d5b5c2afe90788595008e + your patch for bcm2835.
The firmware from this commit 8a14be1364cf01ef79cd29f90f977ef3644233a1 to ensure that everything is working before your test firmware.
version 4942b7633c0ff1af1ee95a51a33b56a9dae47529 (clean) (release) (start).
The kernel was built for armv7l. Maybe this is important?
This setup is working fine.
Next, I've replaced firmware to your files, and got this error:

[    9.556438] ------------[ cut here ]------------
[    9.558056] Loaded X.509 cert 'wens: 61c038651aabdcf94bd0ac7ff06c7248db18c600'
[    9.562044] WARNING: CPU: 2 PID: 1 at kernel/time/timer.c:1738 __run_timers+0x358/0x3a4
[    9.577261] Modules linked in: v3d gpu_sched drm_shmem_helper cfg80211(+) snd_soc_simple_card rfkill snd_soc_simple_card_utils snd_soc_spdif_rx i2c_mux_pinctrl i2c_mux raspberrypi_hwmon i2c_brcmstb bcm2835_unicam i2c_bcm2835 snd_soc_bcm2835_i2s raspberrypi_gpiomem vc4 cdc_acm bcm2835_v4l2(C) bcm2835_codec(C) bcm2835_isp(C) snd_soc_hdmi_codec drm_display_helper drm_dma_helper bcm2835_mmal_vchiq(C) drm_kms_helper rpivid_hevc(C) snd_soc_core vc_sm_cma(C) v4l2_mem2mem snd_compress videobuf2_dma_contig videobuf2_vmalloc snd_pcm_dmaengine videobuf2_memops videobuf2_v4l2 videobuf2_common nvmem_rmem uio_pdrv_genirq uio sch_fq_codel snd_bcm2835(C) snd_pcm snd_timer snd i2c_dev tc358743 v4l2_dv_timings v4l2_fwnode v4l2_async videodev mc cec libcomposite dwc2 roles drm fuse dm_mod drm_panel_orientation_quirks backlight nfnetlink ip_tables x_tables ipv6
[    9.651987] CPU: 2 PID: 1 Comm: systemd Tainted: G         C         6.6.39-2-rpi #1
[    9.659731] Hardware name: BCM2711
[    9.663131]  unwind_backtrace from show_stack+0x18/0x1c
[    9.668362]  show_stack from dump_stack_lvl+0x50/0x68
[    9.673419]  dump_stack_lvl from __warn+0x84/0x114
[    9.678214]  __warn from warn_slowpath_fmt+0x18c/0x194
[    9.683353]  warn_slowpath_fmt from __run_timers+0x358/0x3a4
[    9.689014]  __run_timers from run_timer_softirq+0x28/0x38
[    9.694500]  run_timer_softirq from handle_softirqs+0x114/0x32c
[    9.700425]  handle_softirqs from irq_exit+0x84/0xb4
[    9.705395]  irq_exit from call_with_stack+0x18/0x20
[    9.710364]  call_with_stack from __irq_usr+0x6c/0x80
[    9.715416] Exception stack(0xc881dfb0 to 0xc881dff8)
[    9.720464] dfa0:                                     08a02eab 6ca28000 00ac221c 84438000
[    9.728640] dfc0: 3f264c03 a9f3549c 68336514 bba3209f 5b084fc9 73656465 bee18708 00b29a68
[    9.736814] dfe0: 05b93813 bee186c0 550407dc b6b2a3bc 20010010 ffffffff
[    9.743424] ---[ end trace 0000000000000000 ]---

After few seconds I've got OOM killer:

[   47.403335] kworker/u12:0 invoked oom-killer: gfp_mask=0x440dc0(GFP_KERNEL_ACCOUNT|__GFP_COMP|__GFP_ZERO), order=0, oom_score_adj=0
[   47.415228] CPU: 0 PID: 561 Comm: kworker/u12:0 Tainted: G        WC         6.6.39-2-rpi #1
[   47.423672] Hardware name: BCM2711
[   47.427073]  unwind_backtrace from show_stack+0x18/0x1c
[   47.432312]  show_stack from dump_stack_lvl+0x50/0x68
[   47.437370]  dump_stack_lvl from dump_header+0x54/0x1fc
[   47.442604]  dump_header from oom_kill_process+0x23c/0x248
[   47.448102]  oom_kill_process from out_of_memory+0x100/0x344
[   47.453770]  out_of_memory from __alloc_pages+0xa74/0xf38
[   47.459176]  __alloc_pages from __pmd_alloc+0x44/0x1d4
[   47.464320]  __pmd_alloc from pgd_alloc+0x240/0x270
[   47.469202]  pgd_alloc from mm_init+0xf0/0x270
[   47.473645]  mm_init from alloc_bprm+0x84/0x288
[   47.478176]  alloc_bprm from kernel_execve+0x40/0x1f0
[   47.483228]  kernel_execve from call_usermodehelper_exec_async+0x118/0x188
[   47.490110]  call_usermodehelper_exec_async from ret_from_fork+0x14/0x38
[   47.496813] Exception stack(0xc9269fb0 to 0xc9269ff8)
[   47.501863] 9fa0:                                     00000000 00000000 00000000 00000000
[   47.510038] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   47.518214] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   47.524916] Mem-Info:
[   47.527229] active_anon:11893 inactive_anon:9018 isolated_anon:0
[   47.527229]  active_file:3004 inactive_file:10052 isolated_file:0
[   47.527229]  unevictable:548 dirty:0 writeback:0
[   47.527229]  slab_reclaimable:881 slab_unreclaimable:3647
[   47.527229]  mapped:10157 shmem:2285 pagetables:379
[   47.527229]  sec_pagetables:0 bounce:0
[   47.527229]  kernel_misc_reclaimable:0
[   47.527229]  free:225347 free_pcp:4 free_cma:0
[   47.566174] Node 0 active_anon:47572kB inactive_anon:36072kB active_file:12016kB inactive_file:40208kB unevictable:2192kB isolated(anon):0kB isolated(file):0kB mapped:40628kB dirty:0kB writeback:0kB shmem:9140kB writeback_tmp:0kB kernel_stack:1472kB pagetables:1516kB sec_pagetables:0kB all_unreclaimable? no
[   47.593447] DMA free:824kB boost:0kB min:544kB low:680kB high:816kB reserved_highatomic:0KB active_anon:0kB inactive_anon:0kB active_file:3904kB inactive_file:8kB unevictable:0kB writepending:0kB present:131072kB managed:31208kB mlocked:0kB bounce:0kB free_pcp:56kB local_pcp:4kB free_cma:0kB
[   47.619319] lowmem_reserve[]: 0 0 1024 1024
[   47.623539] DMA: 99*4kB (UE) 44*8kB (UE) 2*16kB (UE) 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 780kB
[   47.634914] 15798 total pagecache pages
[   47.638772] 0 pages in swap cache
[   47.642099] Free swap  = 0kB
[   47.644986] Total swap = 0kB
[   47.647880] 294912 pages RAM
[   47.650777] 262144 pages HighMem/MovableOnly
[   47.655057] 24966 pages reserved
[   47.658317] 0 pages cma reserved
[   47.661579] Tasks state (memory values in pages):
[   47.666297] [  pid  ]   uid  tgid total_vm      rss pgtables_bytes swapents oom_score_adj name
[   47.674989] [    216]     0   216     5273     1632    57344        0          -250 systemd-journal
[   47.684096] [    259]     0   259     7148     1902    61440        0         -1000 systemd-udevd
[   47.693038] [    430]     0   430     3437      928    57344        0             0 rngd
[   47.701189] [    431]   977   431     4469     2752    57344        0             0 systemd-resolve
[   47.710250] [    432]   976   432     5544     1600    57344        0             0 systemd-timesyn
[   47.719327] [    440]    81   440     1669      732    40960        0          -900 dbus-broker-lau
[   47.728395] [    441]    81   441      846      521    32768        0          -900 dbus-broker
[   47.737127] [    443]     0   443     7144      960    61440        0             0 kvmd-fan
[   47.745597] [    445]     0   445     3090     1472    49152        0             0 systemd-logind
[   47.754582] [    448]     0   448      551      480    36864        0         -1000 watchdog
[   47.763060] [    476]     0   476    18983     5408   114688        0             0 python3
[   47.771441] [    481]   967   481     8027     6624    90112        0             0 kvmd-pst
[   47.779895] [    483]     0   483     6589     5469    77824        0             0 kvmd-watchdog
[   47.788804] [    484]   980   484     3490     1920    53248        0             0 systemd-network
[   47.797883] [    487]   961   487     1940      800    36864        0             0 ttyd
[   47.805987] [    488]     0   488     1528     1312    36864        0         -1000 sshd
[   47.814105] [    493]     0   493     1059      608    32768        0             0 agetty
[   47.822394] [    494]     0   494     1059      576    36864        0             0 agetty
[   47.830691] [    516]     0   516     3027     1312    53248        0             0 systemd-hostnam
[   47.839760] [    520]   102   520    15030     1952    77824        0             0 polkitd
[   47.848130] [    527]   967   527     3700     1344    57344        0             0 sudo
[   47.856244] [    533]   968   533     4411     3584    57344        0             0 kvmd
[   47.864394] [    534]   963   534     4411     3616    61440        0             0 kvmd-janus
[   47.873031] [    535]     0   535     4411     3616    57344        0             0 kvmd-nginx-mkco
[   47.882114] [    550]     0   550     7149     1168    61440        0             0 (udev-worker)
[   47.891014] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),cpuset=/,mems_allowed=0,global_oom,task_memcg=/,task=kvmd-pst,pid=481,uid=967
[   47.903756] Out of memory: Killed process 481 (kvmd-pst) total-vm:32108kB, anon-rss:15104kB, file-rss:11392kB, shmem-rss:0kB, UID:967 pgtables:88kB oom_score_adj:0

After a few minutes, the kernel went to kill processes indiscriminately.

6by9 commented 3 months ago

It looks like something might be squiffy with the start4.elf and fixup files, so I'm seeing a failure to create the CMA heap (which also appears to be initialising at 512MB in size).

start_x=1 or start_debug=1 both work OK for me. Could you test with one of those?

Seeing as you've got

0 pages cma reserved

in your OOM log above, I'm guessing something similar is true for you too.

Seeing as the change hasn't touched the generation of the fixups, I suspect it's just a glitch.

mdevaev commented 3 months ago

Thank you. I checked start_x=1 and now it looks working and colors are fine. Will I need to use start_x=1 all the time, or will you fix something else?

But I see some artifacts (in both RGB24 and UYVY). I suppose the reason is 1920x1200, because 1920x1080 is working fine.

I understand that the 1920x1080 resolution is an undocumented feature. But if it is impossible to make the right colors on it, it would be good to get rid of glitches in this mode at least. Previously, there were no glitches and the mode worked well.

6by9 commented 3 months ago

Hopefully the requirement for start_x or start_debug is just a glitch in the build system, but I'll flag it for others before this change gets merged (I don't fully understand the fixup.dat magic).

I'm also at a slight loss over the image artifacts. Other than tweaking the ISP parameters to reflect the more common RGB->YUV matrices, it adds signalling to the codec code to defer generating the H264 headers and sets the colour description fields before presenting the first frame. That means the headers get eg

9.3: sps->vui.video_signal_type_present_flag: 1 
9.2: sps->vui.video_format: 5 
10.7: sps->vui.video_full_range_flag: 0 
10.6: sps->vui.colour_description_present_flag: 1 
10.5: sps->vui.colour_primaries: 1 
11.5: sps->vui.transfer_characteristics: 1 
12.5: sps->vui.matrix_coefficients: 1

inserted for BT709 limited range.

Reverting the kernel change should go back to the old behaviour, but I still see image corruption. Something seems squiffy, so it's a case of backing out each bit of the patch to work out the subtlety.

6by9 commented 3 months ago

Doh, I'd assigned extra flags in, rather than or'ing them in, so we got dodgy behaviour.

New firmware to test. I've not checked whether we've still got the build quirk that requires start_x / start_debug. fw1885_1.zip

mdevaev commented 3 months ago

It seems everything is working fine now. But without start_x=1 it's not booting at all, I didn't event see the boot rainbow.

mdevaev commented 3 months ago

It seems the kernel patch is already merged to upstream, what about the firmware?

popcornmix commented 3 months ago

But without start_x=1 it's not booting at all, I didn't event see the boot rainbow.

is being investigated.

mdevaev commented 3 months ago

Got it.

popcornmix commented 3 months ago

We've pushed it to rpi-update firmware as that build pipeline doesn't seem to have the issue (an oversized fixup4.dat file). We're still looking into why the internal build pipeline produces a different fixup4.dat file.

6by9 commented 3 months ago

FWIW https://github.com/raspberrypi/linux/pull/6283 adds the ability to select H264 levels 5 and 5.1 on the V4L2 encoder, otherwise you can't encode 1920x1200.

any1 commented 2 months ago

For YUV formats it should encode the colourspace information from userspace into the bitstream headers. For RGB formats, it should convert to BT601 limited range for SD resolutions, and BT709 limited range for anything greater than 720x576. That should also be encoded into the bitstream headers.

For the sake of curiosity, what are the benefits of using limited range for this? Wouldn't full-range sRGB/sYCC be better suited for something that is ultimately going to be displayed on a computer monitor?

Also, just for sake of clarity, the input colour space is expected to be sRGB for RGB formatted buffers, yes?

6by9 commented 2 months ago

Limited range is the default for BT601, 709, and 2020 with almost all the video codecs from MPEG-2 to H265. Only JPEGs use full range (with the same CCM as BT601, sometimes referred to as JFIF). The why is probably buried in the history of analogue video that was being replicated by digital. In the absence of explicit headers in the bitstream, players will generally assume limited range.

It's the same quirk with HDMI - all CEA (Consumer Electronics Association) defined modes use limited range signalling in the RGB space on the wire, whilst the DMT modes (defined by VESA and the IT industry) use full range. Why compromise on the range? Again goodness only knows, particularly as this is in the RGB space, so the display has to scale that back up to drive the pixels.

Yes RGB formats expect sRGB. The encoder only deals in YUV420 so it will be converted to 601/709 assuming that.

mdevaev commented 2 months ago

BTW is the subj fix already available in the firmware or you dealing with build pipeline?

6by9 commented 2 months ago

BTW is the subj fix already available in the firmware or you dealing with build pipeline?

Went in 3 weeks ago - https://github.com/raspberrypi/rpi-firmware/commit/9f180c06514a3084f6fe61fe27050fd5e5ce60ba as mentioned in https://github.com/raspberrypi/firmware/issues/1885#issuecomment-2250441275

Build pipeline issues were that the CI system hadn't been updated to use the current scripts (Python 2 to Python 3 change) - all resolved now.

mdevaev commented 2 months ago

Ah, sorry, I missed this comment. So, everything is working great now! Thank you for your hard work :heart:

raspberrypi / firmware

bcm2835-codec: Wrong colors on encoding BGR24 to H.264 #1885