| Message ID | 20220105083107.1930899-4-martin@martin.st |
|---|---|
| State | Accepted |
| Commit | 24b93022fee322c7a17e326e2d162a1fbc3698ee |
| Headers | show |
| Series | [FFmpeg-devel,1/4] Revert "lavc/aarch64: add hevc sao band 8x8 tiling" | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 1/5/2022 5:31 AM, Martin Storsjö wrote: > While this function on its own passes all of fate-hevc, there's > indications that the function might need to handle widths that > aren't a multiple of 8 (noted in commit > f63f9be37c799ddc835af358034630d31fb7db02, which later was > reverted). > --- > libavcodec/aarch64/hevcdsp_init_aarch64.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c b/libavcodec/aarch64/hevcdsp_init_aarch64.c > index c785e46f79..1e40be740c 100644 > --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c > +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c > @@ -75,7 +75,11 @@ av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth) > c->idct_dc[1] = ff_hevc_idct_8x8_dc_8_neon; > c->idct_dc[2] = ff_hevc_idct_16x16_dc_8_neon; > c->idct_dc[3] = ff_hevc_idct_32x32_dc_8_neon; > - c->sao_band_filter[0] = ff_hevc_sao_band_filter_8x8_8_neon; > + // This function is disabled, as it doesn't handle widths that aren't > + // an even multiple of 8 correctly. fate-hevc doesn't exercise that > + // for the current size, but if enabled for bigger sizes, the cases > + // of non-multiple of 8 seem to arise. Can the checkasm test be extended to cover these cases? > +// c->sao_band_filter[0] = ff_hevc_sao_band_filter_8x8_8_neon; > } > if (bit_depth == 10) { > c->add_residual[0] = ff_hevc_add_residual_4x4_10_neon;
On Wed, 5 Jan 2022, James Almer wrote: > > > On 1/5/2022 5:31 AM, Martin Storsjö wrote: >> While this function on its own passes all of fate-hevc, there's >> indications that the function might need to handle widths that >> aren't a multiple of 8 (noted in commit >> f63f9be37c799ddc835af358034630d31fb7db02, which later was >> reverted). >> --- >> libavcodec/aarch64/hevcdsp_init_aarch64.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c > b/libavcodec/aarch64/hevcdsp_init_aarch64.c >> index c785e46f79..1e40be740c 100644 >> --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c >> +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c >> @@ -75,7 +75,11 @@ av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, > const int bit_depth) >> c->idct_dc[1] = ff_hevc_idct_8x8_dc_8_neon; >> c->idct_dc[2] = ff_hevc_idct_16x16_dc_8_neon; >> c->idct_dc[3] = ff_hevc_idct_32x32_dc_8_neon; >> - c->sao_band_filter[0] = > ff_hevc_sao_band_filter_8x8_8_neon; >> + // This function is disabled, as it doesn't handle widths that > aren't >> + // an even multiple of 8 correctly. fate-hevc doesn't exercise > that >> + // for the current size, but if enabled for bigger sizes, the > cases >> + // of non-multiple of 8 seem to arise. > > Can the checkasm test be extended to cover these cases? Yes, ideally - but I'm not that familiar with it (from the decoder point of view, what argument combinations are valid and expected) and I don't quite have bandwidth to take it on right now, so yes - patch welcome. // Martin
On Wed, 5 Jan 2022, Martin Storsjö wrote: > While this function on its own passes all of fate-hevc, there's > indications that the function might need to handle widths that > aren't a multiple of 8 (noted in commit > f63f9be37c799ddc835af358034630d31fb7db02, which later was > reverted). > --- > libavcodec/aarch64/hevcdsp_init_aarch64.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) As fate-hevc still is broken on aarch64, is anyone opposed to pushing these reverts until the things is sorted out? I'm all for fixing things, but we don't need to leave things broken in git master while doing so. // Martin
On Fri, 7 Jan 2022, Martin Storsjö wrote: > On Wed, 5 Jan 2022, Martin Storsjö wrote: > >> While this function on its own passes all of fate-hevc, there's >> indications that the function might need to handle widths that >> aren't a multiple of 8 (noted in commit >> f63f9be37c799ddc835af358034630d31fb7db02, which later was >> reverted). >> --- >> libavcodec/aarch64/hevcdsp_init_aarch64.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) > > As fate-hevc still is broken on aarch64, is anyone opposed to pushing these > reverts until the things is sorted out? Pushed these reverts now, plus I also pushed patch 4/4 to the 5.0 release branch. // Martin
On Fri, 07. Jan 01:30, Martin Storsjö wrote: > On Wed, 5 Jan 2022, Martin Storsjö wrote: > > > While this function on its own passes all of fate-hevc, there's > > indications that the function might need to handle widths that > > aren't a multiple of 8 (noted in commit > > f63f9be37c799ddc835af358034630d31fb7db02, which later was > > reverted). > > --- > > libavcodec/aarch64/hevcdsp_init_aarch64.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > As fate-hevc still is broken on aarch64, is anyone opposed to pushing these > reverts until the things is sorted out? > > I'm all for fixing things, but we don't need to leave things broken in git > master while doing so. > I can set up a patchwork CI runner on the FFmpeg M1 machine to catch these in the future. Kieran, can you set up a user for me? Thanks,
diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c b/libavcodec/aarch64/hevcdsp_init_aarch64.c index c785e46f79..1e40be740c 100644 --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c @@ -75,7 +75,11 @@ av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth) c->idct_dc[1] = ff_hevc_idct_8x8_dc_8_neon; c->idct_dc[2] = ff_hevc_idct_16x16_dc_8_neon; c->idct_dc[3] = ff_hevc_idct_32x32_dc_8_neon; - c->sao_band_filter[0] = ff_hevc_sao_band_filter_8x8_8_neon; + // This function is disabled, as it doesn't handle widths that aren't + // an even multiple of 8 correctly. fate-hevc doesn't exercise that + // for the current size, but if enabled for bigger sizes, the cases + // of non-multiple of 8 seem to arise. +// c->sao_band_filter[0] = ff_hevc_sao_band_filter_8x8_8_neon; } if (bit_depth == 10) { c->add_residual[0] = ff_hevc_add_residual_4x4_10_neon;