Message ID | AS8P250MB074470F07F8B7DEFDA2E73978F5A2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avcodec/dxvenc: Use proper alignment, write endian-independent output | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
On 2/26/2024 8:50 PM, Andreas Rheinhardt wrote: > Fixes the dxv3enc-dxt1 FATE test with UBSan. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I have not actually tested whether the output is actually wrong > on BE systems. Would be nice if someone could. > > libavcodec/dxvenc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/dxvenc.c b/libavcodec/dxvenc.c > index 1ce2b1d014..3f28fb696a 100644 > --- a/libavcodec/dxvenc.c > +++ b/libavcodec/dxvenc.c > @@ -149,7 +149,7 @@ typedef struct DXVEncContext { > } else { \ > op = 0; \ > } \ > - *value |= (op << (state * 2)); \ > + AV_WL32(value, AV_RL32(value) | (op << (state * 2))); \ > state++; \ > } while (0) > > @@ -157,7 +157,7 @@ static int dxv_compress_dxt1(AVCodecContext *avctx) > { > DXVEncContext *ctx = avctx->priv_data; > PutByteContext *pbc = &ctx->pbc; > - uint32_t *value; > + void *value; You should probably also remove the uint32_t* cast in the above macro. > uint32_t color, lut, idx, color_idx, lut_idx, prev_pos, state = 16, pos = 2, op = 0; > > ht_init(ctx->color_lookback_ht);
Hi, On Mon, Feb 26, 2024 at 6:48 PM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Fixes the dxv3enc-dxt1 FATE test with UBSan. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I have not actually tested whether the output is actually wrong > on BE systems. Would be nice if someone could. > > libavcodec/dxvenc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/dxvenc.c b/libavcodec/dxvenc.c > index 1ce2b1d014..3f28fb696a 100644 > --- a/libavcodec/dxvenc.c > +++ b/libavcodec/dxvenc.c > @@ -149,7 +149,7 @@ typedef struct DXVEncContext { > } else { \ > op = 0; \ > } \ > - *value |= (op << (state * 2)); \ > + AV_WL32(value, AV_RL32(value) | (op << (state * 2))); \ > state++; \ > } while (0) > > @@ -157,7 +157,7 @@ static int dxv_compress_dxt1(AVCodecContext *avctx) > { > DXVEncContext *ctx = avctx->priv_data; > PutByteContext *pbc = &ctx->pbc; > - uint32_t *value; > + void *value; > uint32_t color, lut, idx, color_idx, lut_idx, prev_pos, state = 16, pos = 2, op = 0; > > ht_init(ctx->color_lookback_ht); Confirming that this does fix the failing test on a big-endian PowerPC + Altivec virtual machine. Thanks, Sean McGovern
Sean McGovern: > Hi, > > On Mon, Feb 26, 2024 at 6:48 PM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: >> >> Fixes the dxv3enc-dxt1 FATE test with UBSan. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> I have not actually tested whether the output is actually wrong >> on BE systems. Would be nice if someone could. >> >> libavcodec/dxvenc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/dxvenc.c b/libavcodec/dxvenc.c >> index 1ce2b1d014..3f28fb696a 100644 >> --- a/libavcodec/dxvenc.c >> +++ b/libavcodec/dxvenc.c >> @@ -149,7 +149,7 @@ typedef struct DXVEncContext { >> } else { \ >> op = 0; \ >> } \ >> - *value |= (op << (state * 2)); \ >> + AV_WL32(value, AV_RL32(value) | (op << (state * 2))); \ >> state++; \ >> } while (0) >> >> @@ -157,7 +157,7 @@ static int dxv_compress_dxt1(AVCodecContext *avctx) >> { >> DXVEncContext *ctx = avctx->priv_data; >> PutByteContext *pbc = &ctx->pbc; >> - uint32_t *value; >> + void *value; >> uint32_t color, lut, idx, color_idx, lut_idx, prev_pos, state = 16, pos = 2, op = 0; >> >> ht_init(ctx->color_lookback_ht); > > Confirming that this does fix the failing test on a big-endian PowerPC > + Altivec virtual machine. > Thanks for testing, applied the patch with James' suggested modification. - Andreas
diff --git a/libavcodec/dxvenc.c b/libavcodec/dxvenc.c index 1ce2b1d014..3f28fb696a 100644 --- a/libavcodec/dxvenc.c +++ b/libavcodec/dxvenc.c @@ -149,7 +149,7 @@ typedef struct DXVEncContext { } else { \ op = 0; \ } \ - *value |= (op << (state * 2)); \ + AV_WL32(value, AV_RL32(value) | (op << (state * 2))); \ state++; \ } while (0) @@ -157,7 +157,7 @@ static int dxv_compress_dxt1(AVCodecContext *avctx) { DXVEncContext *ctx = avctx->priv_data; PutByteContext *pbc = &ctx->pbc; - uint32_t *value; + void *value; uint32_t color, lut, idx, color_idx, lut_idx, prev_pos, state = 16, pos = 2, op = 0; ht_init(ctx->color_lookback_ht);
Fixes the dxv3enc-dxt1 FATE test with UBSan. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- I have not actually tested whether the output is actually wrong on BE systems. Would be nice if someone could. libavcodec/dxvenc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)