* [FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction
@ 2022-06-30 7:57 Zhao Zhili
2022-06-30 8:31 ` Andreas Rheinhardt
0 siblings, 1 reply; 5+ messages in thread
From: Zhao Zhili @ 2022-06-30 7:57 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Zhao Zhili
From: Zhao Zhili <zhilizhao@tencent.com>
Fixes ticket #9816.
Regression since ed0001482a74b60f3d5bc5.
Prediction value larger than INT32_MAX should be treated as
negative. The code already depends on undefined right shift
behavior before the patch, which doesn't get fixed by the patch.
Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
libavcodec/apedec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
index a7c38bce1b..5f2af2e147 100644
--- a/libavcodec/apedec.c
+++ b/libavcodec/apedec.c
@@ -1198,7 +1198,7 @@ static av_always_inline int predictor_update_filter(APEPredictor64 *p,
p->buf[delayB - 3] * p->coeffsB[filter][3] +
p->buf[delayB - 4] * p->coeffsB[filter][4];
- p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
+ p->lastA[filter] = decoded + ((int32_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
p->filterA[filter] = p->lastA[filter] + ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
sign = APESIGN(decoded);
--
2.35.3
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction
2022-06-30 7:57 [FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction Zhao Zhili
@ 2022-06-30 8:31 ` Andreas Rheinhardt
2022-06-30 8:49 ` Paul B Mahol
0 siblings, 1 reply; 5+ messages in thread
From: Andreas Rheinhardt @ 2022-06-30 8:31 UTC (permalink / raw)
To: ffmpeg-devel
Zhao Zhili:
> From: Zhao Zhili <zhilizhao@tencent.com>
>
> Fixes ticket #9816.
> Regression since ed0001482a74b60f3d5bc5.
>
> Prediction value larger than INT32_MAX should be treated as
> negative. The code already depends on undefined right shift
> behavior before the patch, which doesn't get fixed by the patch.
>
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
> libavcodec/apedec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> index a7c38bce1b..5f2af2e147 100644
> --- a/libavcodec/apedec.c
> +++ b/libavcodec/apedec.c
> @@ -1198,7 +1198,7 @@ static av_always_inline int predictor_update_filter(APEPredictor64 *p,
> p->buf[delayB - 3] * p->coeffsB[filter][3] +
> p->buf[delayB - 4] * p->coeffsB[filter][4];
>
> - p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
> + p->lastA[filter] = decoded + ((int32_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
> p->filterA[filter] = p->lastA[filter] + ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
>
> sign = APESIGN(decoded);
1. Right shifts of negative numbers are implementation-defined, not
undefined. And we rely on the implementation to do the sane thing for
two's complement, so it is defined for us.
2. You are not necessarily treating them as negative, you simply discard
the upper 32 bits and the result could be positive. And you discard the
upper bits before you discard the lowest 10 bits, so the result has
effectively only 22 bits. Is this really intended? If it is, you could
perform the addition predictionA + (predictionB >> 1) in 32 bits.
3. I see one possibility for UB in this: The outermost addition on the
right side is a signed addition, so it could overflow.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction
2022-06-30 8:31 ` Andreas Rheinhardt
@ 2022-06-30 8:49 ` Paul B Mahol
2022-06-30 9:09 ` [FFmpeg-devel] [PATCH v2] " Zhao Zhili
0 siblings, 1 reply; 5+ messages in thread
From: Paul B Mahol @ 2022-06-30 8:49 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Thu, Jun 30, 2022 at 10:31 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Zhao Zhili:
> > From: Zhao Zhili <zhilizhao@tencent.com>
> >
> > Fixes ticket #9816.
> > Regression since ed0001482a74b60f3d5bc5.
> >
> > Prediction value larger than INT32_MAX should be treated as
> > negative. The code already depends on undefined right shift
> > behavior before the patch, which doesn't get fixed by the patch.
> >
> > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > ---
> > libavcodec/apedec.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> > index a7c38bce1b..5f2af2e147 100644
> > --- a/libavcodec/apedec.c
> > +++ b/libavcodec/apedec.c
> > @@ -1198,7 +1198,7 @@ static av_always_inline int
> predictor_update_filter(APEPredictor64 *p,
> > p->buf[delayB - 3] * p->coeffsB[filter][3] +
> > p->buf[delayB - 4] * p->coeffsB[filter][4];
> >
> > - p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA +
> (predictionB >> 1)) >> 10);
> > + p->lastA[filter] = decoded + ((int32_t)((uint64_t)predictionA +
> (predictionB >> 1)) >> 10);
> > p->filterA[filter] = p->lastA[filter] +
> ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
> >
> > sign = APESIGN(decoded);
>
> 1. Right shifts of negative numbers are implementation-defined, not
> undefined. And we rely on the implementation to do the sane thing for
> two's complement, so it is defined for us.
> 2. You are not necessarily treating them as negative, you simply discard
> the upper 32 bits and the result could be positive. And you discard the
> upper bits before you discard the lowest 10 bits, so the result has
> effectively only 22 bits. Is this really intended? If it is, you could
> perform the addition predictionA + (predictionB >> 1) in 32 bits.
> 3. I see one possibility for UB in this: The outermost addition on the
> right side is a signed addition, so it could overflow.
>
Also make sure that you do not break insane ape files, its on one of trac
issues.
>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 5+ messages in thread
* [FFmpeg-devel] [PATCH v2] avcodec/apedec: fix prediction
2022-06-30 8:49 ` Paul B Mahol
@ 2022-06-30 9:09 ` Zhao Zhili
2022-06-30 9:42 ` "zhilizhao(赵志立)"
0 siblings, 1 reply; 5+ messages in thread
From: Zhao Zhili @ 2022-06-30 9:09 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Zhao Zhili
From: Zhao Zhili <zhilizhao@tencent.com>
Regression since ed0001482a74b60f3d5bc5.
Tested with:
./ffmpeg -i foo.ape -f md5 -
for samples in #9816 and #2239.
It gives the same result as before ed0001482a for ticket #9816,
and doesn't break ticket #2239.
Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
libavcodec/apedec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
index a7c38bce1b..f5c0f75f7f 100644
--- a/libavcodec/apedec.c
+++ b/libavcodec/apedec.c
@@ -1198,7 +1198,7 @@ static av_always_inline int predictor_update_filter(APEPredictor64 *p,
p->buf[delayB - 3] * p->coeffsB[filter][3] +
p->buf[delayB - 4] * p->coeffsB[filter][4];
- p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
+ p->lastA[filter] = decoded + ((int32_t)((uint32_t)predictionA + ((int32_t)predictionB >> 1)) >> 10);
p->filterA[filter] = p->lastA[filter] + ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
sign = APESIGN(decoded);
--
2.35.3
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [FFmpeg-devel] [PATCH v2] avcodec/apedec: fix prediction
2022-06-30 9:09 ` [FFmpeg-devel] [PATCH v2] " Zhao Zhili
@ 2022-06-30 9:42 ` "zhilizhao(赵志立)"
0 siblings, 0 replies; 5+ messages in thread
From: "zhilizhao(赵志立)" @ 2022-06-30 9:42 UTC (permalink / raw)
To: FFmpeg development discussions and patches
> On Jun 30, 2022, at 5:09 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
>
> From: Zhao Zhili <zhilizhao@tencent.com>
>
> Regression since ed0001482a74b60f3d5bc5.
>
> Tested with:
> ./ffmpeg -i foo.ape -f md5 -
> for samples in #9816 and #2239.
>
> It gives the same result as before ed0001482a for ticket #9816,
> and doesn't break ticket #2239.
OK, It did break #8918. The compression_level is different between #9816
and #8918, I have no idea if it can be used as the condition, or just
some random bug in the encoders which created those files.
>
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
> libavcodec/apedec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> index a7c38bce1b..f5c0f75f7f 100644
> --- a/libavcodec/apedec.c
> +++ b/libavcodec/apedec.c
> @@ -1198,7 +1198,7 @@ static av_always_inline int predictor_update_filter(APEPredictor64 *p,
> p->buf[delayB - 3] * p->coeffsB[filter][3] +
> p->buf[delayB - 4] * p->coeffsB[filter][4];
>
> - p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
> + p->lastA[filter] = decoded + ((int32_t)((uint32_t)predictionA + ((int32_t)predictionB >> 1)) >> 10);
> p->filterA[filter] = p->lastA[filter] + ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
>
> sign = APESIGN(decoded);
> --
> 2.35.3
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-30 9:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-30 7:57 [FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction Zhao Zhili
2022-06-30 8:31 ` Andreas Rheinhardt
2022-06-30 8:49 ` Paul B Mahol
2022-06-30 9:09 ` [FFmpeg-devel] [PATCH v2] " Zhao Zhili
2022-06-30 9:42 ` "zhilizhao(赵志立)"
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git