Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
Hi, On Fri, Oct 14, 2016 at 2:09 PM, James Zernwrote: > Ronald, > > On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultje > wrote: > > This is intended to workaround bug "665 Integer Divide Instruction May > > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a > > div-by-zero in this codepath, such as reported in Mozilla bug #1293996. > > > > Note that this isn't guaranteed to fix the bug, since a compiler is free > > to reorder instructions that don't depend on each other. However, it > > appears to fix the bug in Firefox, and a similar patch was applied to > > libvpx also (see Chrome bug #599899). > > > > I recently made a few additional changes as this regressed in chrome > [1][2], but just like this change there's no guarantee it won't occur > again. > > [1] https://chromium.googlesource.com/webm/libvpx/+/ > 8b4210940ce4183d4cfded42c323612c0c6d1688 > [2] https://chromium.googlesource.com/webm/libvpx/+/ > 82ea74223771793628dbd812c2fd50afcfb8183a > > > --- > > libavcodec/vp9.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > lgtm (Firefox beta crash reports confirmed that the bug is fixed.) Pushed. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
On Fri, Oct 14, 2016 at 11:54 AM, Ronald S. Bultjewrote: > Hi Michael, > > On Fri, Oct 14, 2016 at 2:31 PM, Michael Niedermayer > wrote: > >> On Fri, Oct 14, 2016 at 08:29:37PM +0200, Michael Niedermayer wrote: >> > On Fri, Oct 14, 2016 at 11:09:30AM -0700, James Zern wrote: >> > > Ronald, >> > > >> > > On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultje >> wrote: >> > > > This is intended to workaround bug "665 Integer Divide Instruction >> May >> > > > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a >> > > > div-by-zero in this codepath, such as reported in Mozilla bug >> #1293996. >> > > > >> > > > Note that this isn't guaranteed to fix the bug, since a compiler is >> free >> > > > to reorder instructions that don't depend on each other. However, it >> > > > appears to fix the bug in Firefox, and a similar patch was applied to >> > > > libvpx also (see Chrome bug #599899). >> > > > >> > > >> > > I recently made a few additional changes as this regressed in chrome >> > > [1][2], but just like this change there's no guarantee it won't occur >> > > again. >> > >> > maybe you can use empty "asm volatile(:::"memory")" statments to >> > prevent unwanted instruction reordering by the compiler >> > never tried something like this so dunno, also it would be specific >> > to gcc compatible compilers but should not be architecture specific >> >> thinking again, why dont you write the function in asm for x86 ? >> this would take the compiler out of the equation > > > I think the primary reason is that "this seems to work". Don't forget that > the bug is in the hardware, not in the code, so I don't want to make the > code needlessly (well... maybe that's debatable) complicated for a problem > that isn't really ours... > > But I guess I'm open to hearing everyone else's opinion on this - if people > want me to make the workaround more persistent I can work on that. > That ended up being my view mostly due to the fact that I didn't have access to the hardware. Even with assembly there is a timing/scheduling element, so it may be less fragile, but I think the bug could still occur. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
Hi Michael, On Fri, Oct 14, 2016 at 2:31 PM, Michael Niedermayerwrote: > On Fri, Oct 14, 2016 at 08:29:37PM +0200, Michael Niedermayer wrote: > > On Fri, Oct 14, 2016 at 11:09:30AM -0700, James Zern wrote: > > > Ronald, > > > > > > On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultje > wrote: > > > > This is intended to workaround bug "665 Integer Divide Instruction > May > > > > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a > > > > div-by-zero in this codepath, such as reported in Mozilla bug > #1293996. > > > > > > > > Note that this isn't guaranteed to fix the bug, since a compiler is > free > > > > to reorder instructions that don't depend on each other. However, it > > > > appears to fix the bug in Firefox, and a similar patch was applied to > > > > libvpx also (see Chrome bug #599899). > > > > > > > > > > I recently made a few additional changes as this regressed in chrome > > > [1][2], but just like this change there's no guarantee it won't occur > > > again. > > > > maybe you can use empty "asm volatile(:::"memory")" statments to > > prevent unwanted instruction reordering by the compiler > > never tried something like this so dunno, also it would be specific > > to gcc compatible compilers but should not be architecture specific > > thinking again, why dont you write the function in asm for x86 ? > this would take the compiler out of the equation I think the primary reason is that "this seems to work". Don't forget that the bug is in the hardware, not in the code, so I don't want to make the code needlessly (well... maybe that's debatable) complicated for a problem that isn't really ours... But I guess I'm open to hearing everyone else's opinion on this - if people want me to make the workaround more persistent I can work on that. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
On Fri, Oct 14, 2016 at 11:31 AM, Michael Niedermayerwrote: > On Fri, Oct 14, 2016 at 08:29:37PM +0200, Michael Niedermayer wrote: >> On Fri, Oct 14, 2016 at 11:09:30AM -0700, James Zern wrote: >> > Ronald, >> > >> > On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultje >> > wrote: >> > > This is intended to workaround bug "665 Integer Divide Instruction May >> > > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a >> > > div-by-zero in this codepath, such as reported in Mozilla bug #1293996. >> > > >> > > Note that this isn't guaranteed to fix the bug, since a compiler is free >> > > to reorder instructions that don't depend on each other. However, it >> > > appears to fix the bug in Firefox, and a similar patch was applied to >> > > libvpx also (see Chrome bug #599899). >> > > >> > >> > I recently made a few additional changes as this regressed in chrome >> > [1][2], but just like this change there's no guarantee it won't occur >> > again. >> >> maybe you can use empty "asm volatile(:::"memory")" statments to >> prevent unwanted instruction reordering by the compiler >> never tried something like this so dunno, also it would be specific >> to gcc compatible compilers but should not be architecture specific > > thinking again, why dont you write the function in asm for x86 ? > this would take the compiler out of the equation > That could work, I started with a portion in assembly before making the C changes, but didn't follow through with it. > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The educated differ from the uneducated as much as the living from the > dead. -- Aristotle > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
On Fri, Oct 14, 2016 at 08:29:37PM +0200, Michael Niedermayer wrote: > On Fri, Oct 14, 2016 at 11:09:30AM -0700, James Zern wrote: > > Ronald, > > > > On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultje> > wrote: > > > This is intended to workaround bug "665 Integer Divide Instruction May > > > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a > > > div-by-zero in this codepath, such as reported in Mozilla bug #1293996. > > > > > > Note that this isn't guaranteed to fix the bug, since a compiler is free > > > to reorder instructions that don't depend on each other. However, it > > > appears to fix the bug in Firefox, and a similar patch was applied to > > > libvpx also (see Chrome bug #599899). > > > > > > > I recently made a few additional changes as this regressed in chrome > > [1][2], but just like this change there's no guarantee it won't occur > > again. > > maybe you can use empty "asm volatile(:::"memory")" statments to > prevent unwanted instruction reordering by the compiler > never tried something like this so dunno, also it would be specific > to gcc compatible compilers but should not be architecture specific thinking again, why dont you write the function in asm for x86 ? this would take the compiler out of the equation [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
On Fri, Oct 14, 2016 at 11:09:30AM -0700, James Zern wrote: > Ronald, > > On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultjewrote: > > This is intended to workaround bug "665 Integer Divide Instruction May > > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a > > div-by-zero in this codepath, such as reported in Mozilla bug #1293996. > > > > Note that this isn't guaranteed to fix the bug, since a compiler is free > > to reorder instructions that don't depend on each other. However, it > > appears to fix the bug in Firefox, and a similar patch was applied to > > libvpx also (see Chrome bug #599899). > > > > I recently made a few additional changes as this regressed in chrome > [1][2], but just like this change there's no guarantee it won't occur > again. maybe you can use empty "asm volatile(:::"memory")" statments to prevent unwanted instruction reordering by the compiler never tried something like this so dunno, also it would be specific to gcc compatible compilers but should not be architecture specific [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
Ronald, On Fri, Oct 14, 2016 at 10:01 AM, Ronald S. Bultjewrote: > This is intended to workaround bug "665 Integer Divide Instruction May > Cause Unpredictable Behavior" on some early AMD CPUs, which causes a > div-by-zero in this codepath, such as reported in Mozilla bug #1293996. > > Note that this isn't guaranteed to fix the bug, since a compiler is free > to reorder instructions that don't depend on each other. However, it > appears to fix the bug in Firefox, and a similar patch was applied to > libvpx also (see Chrome bug #599899). > I recently made a few additional changes as this regressed in chrome [1][2], but just like this change there's no guarantee it won't occur again. [1] https://chromium.googlesource.com/webm/libvpx/+/8b4210940ce4183d4cfded42c323612c0c6d1688 [2] https://chromium.googlesource.com/webm/libvpx/+/82ea74223771793628dbd812c2fd50afcfb8183a > --- > libavcodec/vp9.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > lgtm > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c > index cb2a4a2..3b72149 100644 > --- a/libavcodec/vp9.c > +++ b/libavcodec/vp9.c > @@ -3705,11 +3705,10 @@ static av_always_inline void adapt_prob(uint8_t *p, > unsigned ct0, unsigned ct1, > if (!ct) > return; > > +update_factor = FASTDIV(update_factor * FFMIN(ct, max_count), max_count); > p1 = *p; > -p2 = ((ct0 << 8) + (ct >> 1)) / ct; > +p2 = int64_t) ct0) << 8) + (ct >> 1)) / ct; > p2 = av_clip(p2, 1, 255); > -ct = FFMIN(ct, max_count); > -update_factor = FASTDIV(update_factor * ct, max_count); > > // (p1 * (256 - update_factor) + p2 * update_factor + 128) >> 8 > *p = p1 + (((p2 - p1) * update_factor + 128) >> 8); > -- > 2.8.1 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] vp9: change order of operations in adapt_prob().
This is intended to workaround bug "665 Integer Divide Instruction May Cause Unpredictable Behavior" on some early AMD CPUs, which causes a div-by-zero in this codepath, such as reported in Mozilla bug #1293996. Note that this isn't guaranteed to fix the bug, since a compiler is free to reorder instructions that don't depend on each other. However, it appears to fix the bug in Firefox, and a similar patch was applied to libvpx also (see Chrome bug #599899). --- libavcodec/vp9.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c index cb2a4a2..3b72149 100644 --- a/libavcodec/vp9.c +++ b/libavcodec/vp9.c @@ -3705,11 +3705,10 @@ static av_always_inline void adapt_prob(uint8_t *p, unsigned ct0, unsigned ct1, if (!ct) return; +update_factor = FASTDIV(update_factor * FFMIN(ct, max_count), max_count); p1 = *p; -p2 = ((ct0 << 8) + (ct >> 1)) / ct; +p2 = int64_t) ct0) << 8) + (ct >> 1)) / ct; p2 = av_clip(p2, 1, 255); -ct = FFMIN(ct, max_count); -update_factor = FASTDIV(update_factor * ct, max_count); // (p1 * (256 - update_factor) + p2 * update_factor + 128) >> 8 *p = p1 + (((p2 - p1) * update_factor + 128) >> 8); -- 2.8.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel