Opened 4 years ago

Closed 4 years ago

#1320 closed defect (fixed)

Range of values in residual modification process

Reported by: peterderivaz Owned by:
Priority: minor Milestone: HM-16.3
Component: HM Version: HM-16.2
Keywords: Cc: davidf, joel, teruhiko, jct-vc@…

Description

I wonder if there should be some limit on the values generated by the residual modification process?

At the moment there does not appear to be any constraint, so if cu_transquant_bypass_flag is equal to 1, equation 8-267 allows r[x][y] to be as large as CoeffMax (215 -1 for Main 4:4:4 10).

The residual modification process 8-6-5 then computes a cumulative sum of up to nTbS of these values using equation 8-292.

The final value can therefore be nTbS*CoeffMax.

The reference code assumes that the values are at most 16bits unless compiled with a target of all_highbitdepth.

This means that we can make a legal Main 4:4:4 10 stream that decodes differently depending on whether the reference code is compiled in highbitdepth mode or not.

It seems to makes little sense for the residual values to be allowed to be this large as they will just be clipped during the reconstruction process.

Attachments (1)

fix_1320_rdpcm_accumulator.patch (6.3 KB) - added by karlsharman 4 years ago.
Possible fix for software.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc davidf joel teruhiko jct-vc@… added

comment:2 Changed 4 years ago by karlsharman

No additional clipping has been explicitly added to the RDPCM path, as discussed during presentation of JCTVC-O0066, with meeting notes found in the BoG report JCTVC-O0352.

Once the residual is combined with the prediction it is clipped to the valid video output range; this permits implementations to include clipping to the residual path to limit intermediate values.

This trac-report is perhaps identifying a missing clip in the HM decoder implementation (with the current calculation overflowing in extreme cases) ?

In invRdpcmNxN, the residual values are written to the 16-bit Pel array without clipping. Potentially, for n-bit source data, the values might reach n+log2(32)+sign, which may exceed the 16-bit values. Therefore clipping to 16-bit (size of Pel) should be added to the software. This clipping is not needed in the specification, and the software would be conformant, because if values larger than 16-bit are encountered, these will be clipped to n-bit at the final combination with the prediction.

comment:3 Changed 4 years ago by peterderivaz

Thanks for the links to the meeting notes.

I am not sure I quite understand how an implementation can safely include clipping in the accumulator.

Suppose the inputs are 30000,30000,-30000

If we clip the accumulator at 32767 then the final answer will be 2767, while if we compute with full precision won't the final answer be 30000?

comment:4 Changed 4 years ago by karlsharman

The accumulator part cannot be clipped. But the final block of residuals can be.
The software probably needs to change the order that the residuals are combined so that a 32-bit accumulator can be used. On storing to the residual array (which is then not subsequently used by the invRDPCM process), the values can be clipped to fit within the Pel.

comment:5 Changed 4 years ago by karlsharman

  • Component changed from RExt text to HM
  • Milestone set to HM-next
  • Type changed from enhancement to defect
  • Version changed from RExt D7 (Q1005) v8 to HM-16.2

Changed 4 years ago by karlsharman

Possible fix for software.

comment:6 Changed 4 years ago by karlsharman

  • Milestone changed from HM-next to HM-16.3
  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r4228. No effect on CTCs (except perhaps run-time).

Note: See TracTickets for help on using tickets.

This list contains all users that will be notified about changes made to this ticket.

These roles will be notified: Reporter, Owner, Subscriber, Participant

  • David Flynn(Subscriber)
  • jct-vc@…(Subscriber)
  • Joel(Subscriber)
  • Karl Sharman(Participant, Always)
  • Karsten Suehring(Always)
  • Peter de Rivaz(Reporter, Participant)
  • Teruhiko Suzuki(Subscriber)