Issue 1797 in webm: vp9_intrapred_test: extend above[] with random, not duplicate, values

[jz… via monorail]

Status: Untriaged
Owner: jz...@google.com
CC: georg...@arm.com
Labels: Type-Bug Pri-2

New issue 1797 by jz...@google.com: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797

v1.13.0-182-g5b05f6f3a

This more closely matches the behavior of the decoder where the above row may refer to the reference frame if available [1].

For context, see some of the recent Arm changes and reverts [2].

Unfortunately this was lost along the way [3][4] and there appears to be some issues in the ssse3 which don't surface in the test vectors. Note AV1 still has the original loop which didn't do extension [5].

[1] https://chromium.googlesource.com/webm/libvpx/+/v1.13.0-182-g5b05f6f3a/vp9/common/vp9_reconintra.c#350
[2] https://chromium-review.googlesource.com/c/webm/libvpx/+/4303718
[3] 5b05f6f3a Merge changes Ide512788,I77c7abae into main
[4] https://crbug.com/webm/1316
[5] https://aomedia.googlesource.com/aom/+/3b1b26e9736b52ede90bf3974513c73991ac6668/test/intrapred_test.cc#80

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[jz… via monorail]

Comment #1 on issue 1797 by jz...@google.com: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797#c1

> For context, see some of the recent Arm changes and reverts:

7478b7e4e Revert "Implement highbd_d63_predictor using Neon"

The original loop would have caught this one.

[Git Watcher via monorail]

Comment #2 on issue 1797 by Git Watcher: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797#c2

The following revision refers to this bug:
https://chromium.googlesource.com/webm/libvpx/+/100ca0356ddf67e92da35699d92bc180429d0bc1

commit 100ca0356ddf67e92da35699d92bc180429d0bc1
Author: George Steed <george...@arm.com>
Date: Fri Mar 17 20:00:24 2023

Randomize second half of above_row_ in intrapred tests for Neon

The existing tests duplicate `above_row_[block_size - 1]` after the
first `block_size` elements, which can lead to tests incorrectly passing
due to differing behaviour when calculating the average for the last
elements of the output.

This change adjusts the above array setup to be fully random instead,
allowing us to catch such issues here rather than in other larger tests
like the external MD5 tests.

It doesn't appear that other architectures are fully clean with this
change so restrict it to just Neon for now until they are fixed.

Bug: webm:1797
Change-Id: If83ff1adbf1e8d30f2a92474d7186c65840a5d0b

[modify] https://crrev.com/100ca0356ddf67e92da35699d92bc180429d0bc1/test/vp9_intrapred_test.cc

[Git Watcher via monorail]

Comment #3 on issue 1797 by Git Watcher: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797#c3

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libvpx/+/911d6e165eb19e03ec1532fa20098b10ad402e39

commit 911d6e165eb19e03ec1532fa20098b10ad402e39
Author: George Steed <george...@arm.com>
Date: Fri Mar 17 19:55:17 2023

Allow non-uniform above array in d63 predictor Neon impl

The existing standard bitdepth implementation doesn't appear to manifest
as a failure in any of the predictor or MD5 tests, but it does rely on
the predictor tests filling the second `bs` elements of the `above`
input array with copies of `above[bs - 1]` in order to match the C
implementation.

This patch adjusts the Neon implementation to correctly match the C
implementation in the case where the elements of the `above` array all
differ.

The geomean of performance for the predictor is approximately a 2%
slowdown compared to the previous vectorized implementation. This is
still considerably faster than the unspecialized naive C implementation.

Bug: webm:1797
Change-Id: I8fb00a154288d54b24a72a7ff63c816bdcf3aca3

[modify] https://crrev.com/911d6e165eb19e03ec1532fa20098b10ad402e39/vpx_dsp/arm/intrapred_neon.c

[Git Watcher via monorail]

Comment #4 on issue 1797 by Git Watcher: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797#c4

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libvpx/+/3eb3781589d30874634cab8952dec4ea883eb82a

commit 3eb3781589d30874634cab8952dec4ea883eb82a
Author: George Steed <george...@arm.com>
Date: Fri Mar 17 17:59:26 2023

Allow non-uniform above array in d45 predictor Neon impl

The existing implementation doesn't appear to manifest as a failure in

any of the predictor or MD5 tests, but it does rely on the predictor
tests filling the second `bs` elements of the `above` input array with
copies of `above[bs - 1]` in order to match the C implementation.

This patch adjusts the Neon implementation to correctly match the C
implementation in the case where the elements of the `above` array all
differ.

Performance of the predictor is mostly unchanged, except for the 32x32
block size where it appears to have gotten about 40% faster when
compiled with clang-15.

Bug: webm:1797
Change-Id: Iaad58e77c5467307a3c80d6989b7cf2988e09311

[modify] https://crrev.com/3eb3781589d30874634cab8952dec4ea883eb82a/vpx_dsp/arm/intrapred_neon.c

[Git Watcher via monorail]

Comment #5 on issue 1797 by Git Watcher: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797#c5

The following revision refers to this bug:

https://chromium.googlesource.com/webm/libvpx/+/25825f6a78a267f99c4c6ba7988fc4d79c8cb19d

commit 25825f6a78a267f99c4c6ba7988fc4d79c8cb19d
Author: George Steed <george...@arm.com>
Date: Thu Mar 09 23:46:31 2023

Allow non-uniform above array in highbd d45 predictor Neon impl

The existing implementation doesn't appear to manifest as a failure in
any of the predictor or MD5 tests, but it does rely on the predictor
tests filling the second `bs` elements of the `above` input array with
copies of `above[bs - 1]` in order to match the C implementation.

This patch adjusts the Neon implementation to correctly match the C
implementation in the case where the elements of the `above` array all
differ.

Performance of the predictor is mostly unchanged, except for the 16x16
block size where it appears to have gotten marginally faster across most
compiler/micro-architecture combinations.

Bug: webm:1797
Change-Id: Iac166d6047316c0382e0f2790ce780fc99674b43

[modify] https://crrev.com/25825f6a78a267f99c4c6ba7988fc4d79c8cb19d/vpx_dsp/arm/highbd_intrapred_neon.c

[taay2… via monorail]

Comment #8 on issue 1797 by taay2...@gmail.com: vp9_intrapred_test: extend above[] with random, not duplicate, values
https://bugs.chromium.org/p/webm/issues/detail?id=1797#c8

please explain why I was the target of this project