Opened 7 weeks ago

Closed 3 weeks ago

#1372 closed defect (fixed)

Incorrect top-left and bottom-right ALF padding

Reported by: bheng Owned by:
Priority: minor Milestone:
Component: spec Version: VVC D10 vG
Keywords: Cc: ksuehring, bbross, XiangLi, fbossen, jvet@…

Description

In Sections 8.8.5.2, 8.8.5.3, 8.8.5.4, and 8.8.5.7, the variable hx is defined as a one-dimensional variable. Specifically, the variable hx appears to depend only on location x, with no dependence on location y.

However, for the top-left and bottom-right padding to work correctly (end of Section 8.8.5.6), the values of hx vary depending on the y position as well.

I believe hx for all of those sections should be specified as a 2-dimensional matrix with both an x and a y index.

Change history (5)

comment:1 Changed 5 weeks ago by bbross

Thanks for bringing that up but I am not sure whether I can follow.

hx currently only depends on depends on x as well as vy only depends on y. In 8.8.5.6 (hx, vy) are passed as location (x, y) and both x and y are checked for top-left and bottom-right.
How should hx be defined as an array depending on y as well and how should it be addressed in 8.8.5.6?

Anyone familiar with ALF padding likes to comment on this?

comment:2 Changed 5 weeks ago by bheng

I think 8.8.5.6 is fine. It takes in a (x,y) location and modifies it. The problem is in the other sections.

In 8.8.5.2 for example, I think fixing it right would require replacing the 1-D variable hx with a 7x7 variable h[x][y]. But that notation would get somewhat ugly in the large equations like (1437).

As an alternative, I think it might be close enough if the text only refers to h and v jointly, as locations, and never as two independent variables.

For example, combine (1434) and (1435) into a single equation.

(hx+i, vy+j) = ( Clip3(...), Clip3(...) )

And replace the text:

"The variables hx + i and vy + j are modified by ..."

with:

"The locations (hx+i, vy+j) are modified by ..."

As well as similar changes in 8.8.5.3, 8.8.5.4, and 8.8.5.7.

comment:3 Changed 5 weeks ago by jlchen

Hi, is the first suggested change editorial?

The second change (as following) seems good to me. It makes the text more precise compared to the original text.

replacing the text:
"The variables hx + i and vy + j are modified by ..."
with:
"The locations (hx+i, vy+j) are modified by ..."

comment:4 Changed 4 weeks ago by fbossen

I think Brian is correct. The array h_x should be replaced by an array h_(x,y). This isn't just editorial as it fixes an actual bug in the spec (such as to match the software and I believe the intent as well).

The following should work in 8.8.5.2:

– The locations ( h_(x + i, y + j), v_(y + j) ) for each of the corresponding luma samples ( x, y ) inside the given array recPicture of luma samples with i, j = −3..3 are derived as follows:
( h_(x + i, y + j), v_(y + j) ) = ( Clip3( 0, pps_pic_width_in_luma_samples − 1, xCtb + x + i ), Clip3( 0, pps_pic_height_in_luma_samples − 1, yCtb + y + j ) )

– The locations ( h_(x + i, y + j), v_(y + j) ) with i, j = −3..3 are modified by invoking the ALF sample padding process as specified in subclause 8.8.5.6 with ( xCtb, yCtb ), ( h_(x + i, y + j), v_(y + j) ), the variable isChroma set equal to 0, clipLeftPos, clipRightPos, clipTopPos, clipBottomPos, clipTopLeftFlag and clipBotRightFlag as inputs, and ( h_(x + i, y + j), v_(y + j) ) as output.

In the above v_(y+j) may be passed to / modified by 8.8.5.6 multiple times, but I think that's ok.

All the references to [h_a][v_b] in (1436) and (1437) are changed to [h_(a,b)][v_b].

(and as Brian pointed out, similar changes are needed in 8.8.5.3, 8.8.5.4 and 8.8.5.7)

comment:5 Changed 3 weeks ago by bbross

  • Resolution set to fixed
  • Status changed from new to closed

Fixed as discussed/suggested in JVET-T0110-v2.

Note: See TracTickets for help on using tickets.