Fix motion compensation for bit depths 13 to 16 - #543
felixbuenemann wants to merge 1 commit into
Conversation
Inter prediction was wrong for every bit depth above 12: from the first inter picture on, essentially every sample differed from HM, with errors up to the full sample range. Intra-only streams were fine, so this went unnoticed behind the IDST clipping bug fixed in strukturag#538. Two things in the sample interpolation broke down at 13 bit: - Both put_qpel_fallback and put_epel_hv_fallback used shift1 = BitDepth-8. (8.5.3.3.3.2) and (8.5.3.3.3.3) as amended by the Range Extensions say shift1 = Min(4, BitDepth-8); the v1 formula only agrees up to 12 bit. - With shift1 capped at 4, the filter intermediates and the predicted samples (predSamplesLX, 8.5.3.3.4) need up to 20 bits, but everything in the MC path was int16_t: mcbuffer, predSamplesL/C in motion.cc, the full-sample copies (refPic << shift3, with shift3 = 2 there), and the inputs of the weighted sample prediction. Widening all of it to 32 bits would double the intermediate bandwidth for 9 to 12 bit as well and change the SIMD function signatures, so instead the intermediate type becomes a template parameter of the fallback functions and is selected once per prediction block in generate_inter_prediction_samples(): int16_t through the acceleration table as before for bit depths up to MC_MAX_BIT_DEPTH_INT16 (12), int32_t above that. The int32_t variants are reached through int32_t overloads of the acceleration_functions wrappers, which keep the per-plane 8/16-bit sample split: luma and chroma bit depths are independent, so an 8-bit plane can sit next to a 14-bit one. No acceleration table entries are added for them, as there is no SIMD for any >8-bit MC on any platform; motion.cc stays a plain client of the wrappers and its 8..12-bit path compiles to the same code as before. The int32_t prediction sample set is twice the size of the int16_t one (96 KB) and is kept in per-thread storage rather than on the stack, so the stack requirement of the common path does not grow. Also fixes the mid-grey fill used on the error paths, which assumed shift3 == 14-bd and so was wrong above 12 bit. Verified against HM 18.0 built with RExt__HIGH_BIT_DEPTH_SUPPORT on 640x360 4:2:0 streams from a 16-bit-shifted 10-bit source: random access (hierarchical B, GOP 16) is now bit-exact at every bit depth from 8 to 16, as is low delay with weighted bi-prediction at 8, 13 and 16 bit, with and without high_precision_offsets_enabled_flag, 16-bit random access with implicit/explicit RDPCM, persistent Rice adaptation and transform-skip context enabled, and mixed luma/chroma depths of 8/14, 14/8 and 10/13 bit. Before, 13 to 16 bit differed on 3.4M of 3.7M luma samples per stream and the mixed-depth streams were wrong in whichever plane was above 12 bit. 8-bit testdata/girlshy.h265 decodes identically. Decode speed is unchanged within noise at every depth (256-frame low-delay streams, best of 5).
|
@farindk The code fixes the problem, but it feels too large a change to me. Do you have an idea how we could shrink it? |
Inter prediction above 12 bit decoded wrong from the first P/B picture on. Two things in the fractional sample interpolation break at 13 bit: - put_qpel_fallback() and put_epel_hv_fallback() used shift1 = BitDepth-8. The Range Extensions changed 8.5.3.3.3.2 / 8.5.3.3.3.3 to shift1 = Min(4, BitDepth-8); the v1 formula agrees only up to 12 bit. Fractional-position samples came out 2^(BitDepth-12) too small. - The intermediate prediction samples (predSamplesLX) have max(14, BitDepth+2) bits plus the overshoot of the interpolation filters, but the whole MC path stored them in int16_t (mcbuffer, predSamplesL/C, the full-sample "ref << shift3" copies, the weighted-prediction inputs). The full-sample copy overflows from 14 bit on, fractional positions from 13 bit on. The 16-bit pixel kernels are now templates on the intermediate sample type. Up to MC_MAX_BIT_DEPTH_INT16 (12) they are instantiated with int16_t and reached through the existing acceleration table entries, so nothing changes there. Above it, new "_16_32" table entries with int32_t intermediates are used. The choice is made per colour plane, since luma and chroma bit depths are independent; an 8-bit plane next to a 14-bit one keeps its int16_t path and its SIMD kernels. The int32_t qpel entry is a single generic kernel for all fractional positions instead of 16 specialized copies; that path is not performance critical and the copies would cost ~50 KB of code. generate_inter_prediction_samples() is restructured into a per-plane helper templated on the intermediate type. This also removes the triplicated luma/Cb/Cr calls, reduces the stack usage of the prediction sample buffers (one plane at a time: 16 KB / 32 KB instead of 48 KB), fixes the mid-grey fill on the error path for BitDepth > 12, and makes the REFERENCE_IMAGE_CHROMA_FORMAT_DOES_NOT_MATCH warning reachable (it was shadowed by the size check). The helper is force-inlined (new LIBDE265_ALWAYS_INLINE in util.h) so that the buffer is set up once per PB and not once per plane; without it the 8-bit path executed 0.9% more instructions, with it 0.3% (callgrind, testdata/girlshy.h265). Wall-clock on a 1080p 8-bit stream is unchanged within noise, the SSE kernels are still the ones executing, and the library text grows by 21 KB. Verified against HM 18 (HIGH_BITDEPTH build): bit-exact at 10, 12, 13, 14 and 16 bit for random access (hierarchical B), weighted P and B prediction, with and without high_precision_offsets_enabled_flag, for mixed luma/chroma depths 8/14, 14/8, 10/13, 13/10, 16/9, and for 4:2:2, 4:4:4 and 4:0:0. Bit-exact against ffmpeg for 4:2:0/4:2:2/4:4:4/4:0:0 at 8, 10 and 12 bit. Output identical to the previous code on 2400 fuzzed streams up to 12 bit and on streams with dropped slice NAL units. testdata/girlshy.h265 is unchanged, with and without --noaccel; ASan/UBSan clean. The same defect was diagnosed and fixed independently in PR #543 by Felix Bünemann, with the same two root causes and a template-based fix along the same lines, verified against HM at 640x360 for bit depths 8 to 16 and on 4K content. This commit takes the per-plane variant; the name of the MC_MAX_BIT_DEPTH_INT16 constant is borrowed from that PR. Co-authored-by: Felix Bünemann <felix.buenemann@gmail.com>
|
Thank you. I have also directed my AI to fix this, ending up with a similar patch with the same root causes. We have compared the two implementations and distilled an implementation combining ideas from both. It has a few advantages to the original PR:
Both implementations are bit-exact against HM and produce identical output on every high-bit-depth test stream. Thus, I have chosen the combined implementation and credited you as a Co-author. If you have some real 16bit video streams at hand, it would be good if you can confirm that everything works. |
|
@farindk Thanks! I tested Netflix_ToddlerFountain from https://media.xiph.org/video/derf/ bit-shifted from 10-Bit to 16-Bit and encoded with HM with TemporalFilter disabled (current master HM temporal filter is broken at 16-Bit) and it decodes fine with libde265 master@b85929dc. |
|
Thanks for cross-checking! |
Follow-up to #538. Intra-only 16-bit streams decode correctly since that fix, but any inter picture above 12 bit was still wrong from the first B/P frame on (essentially every sample off, errors up to the full range). This makes 13–16 bit work with motion compensation too, including weighted prediction and mixed luma/chroma bit depths.
Cause. Two things in the sample interpolation break at 13 bit:
put_qpel_fallbackandput_epel_hv_fallbackusedshift1 = BitDepth-8. The Range Extensions changed (8.5.3.3.3.2)/(8.5.3.3.3.3) toshift1 = Min(4, BitDepth-8); the v1 formula agrees only up to 12 bit.shift1capped at 4, the filter intermediates andpredSamplesLXneed up to 20 bits, but the whole MC path wasint16_t(mcbuffer,predSamplesL/C, the full-sampleref << shift3copies, the weighted-prediction inputs).Fix. Rather than widening everything to 32 bit (which would cost bandwidth for 9–12 bit and change the SIMD signatures), the intermediate type is a template parameter of the fallback functions, chosen once per PB in
generate_inter_prediction_samples():int16_tthrough the acceleration table exactly as before up toMC_MAX_BIT_DEPTH_INT16(12),int32_tabove that. Theint32_tvariants are reached throughint32_toverloads of the existingacceleration_functionswrappers, which keep the per-plane 8/16-bit sample split (luma and chroma depths are independent; an 8-bit plane can sit next to a 14-bit one). No table entries are added for them — there is no SIMD for any >8-bit MC on any platform — and the 8–12-bit path compiles to the same code as on master. The 96 KBint32_tprediction-sample set lives in per-thread storage so the stack requirement of the common path does not grow. Also fixes the mid-grey error-path fill, which assumedshift3 == 14-bd.Verification against HM 18.0 built with
RExt__HIGH_BIT_DEPTH_SUPPORT(HM's MCTF pre-filter disabled — it corrupts 16-bit sources — andExtendedPrecisionoff, as libde265 does not implement it). All byte-exact vs the HM decoder unless noted:Also bit-exact: low delay with weighted bi-prediction at 8/13/16 bit, with and without
high_precision_offsets_enabled_flag; 16-bit random access with implicit/explicit RDPCM, persistent Rice adaptation and transform-skip context on; mixed luma/chroma depths 8/14, 14/8, 10/13 (master was wrong in whichever plane was >12 bit);--noaccel;testdata/girlshy.h265identical to master.4K (Netflix ToddlerFountain 4096×2160, first 43 frames, originally 10-Bit, low delay B, WPP):
All within ±3 % run-to-run noise; 13 bit decodes at the same speed as 12 bit. *master decodes garbage at 16 bit, so its timing is not a baseline; on this PR 16 bit runs at 10.3 / 51.6 fps.
Unrelated things noticed on the way, left for separate issues:
transform_skip_rotation_enabled_flaggives small chroma-only errors in inter pictures even at 8 bit, anddec265 -conly reports a picture-hash mismatch on the last picture (read_slice_NAL()discardsdecode_some()'s error).The code in this PR was created with the help of an LLM.
Before/After: