diff mbox

[FFmpeg-devel,1/3] avcodec/h264_cabac: Fix CABAC+8x8dct in 4:4:4

Message ID 1497556233-65009-1-git-send-email-rsbultje@gmail.com
State Accepted
Commit 840b41b2a643fc8f0617c0370125a19c02c6b586
Headers show

Commit Message

Ronald S. Bultje June 15, 2017, 7:50 p.m. UTC 
From: Anton Mitrofanov <BugMaster@narod.ru>

Use the correct ctxIdxInc calculation for coded_block_flag.
Keep old behavior for old versions of x264 for backward compatibility.

Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
---
 libavcodec/h264_cabac.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

Comments

Michael Niedermayer June 16, 2017, 4:46 p.m. UTC | #1 
On Thu, Jun 15, 2017 at 03:50:31PM -0400, Ronald S. Bultje wrote:
> From: Anton Mitrofanov <BugMaster@narod.ru>
> 
> Use the correct ctxIdxInc calculation for coded_block_flag.
> Keep old behavior for old versions of x264 for backward compatibility.
> 
> Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
> ---
>  libavcodec/h264_cabac.c | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)

patchset should be ok if tested against some reference

also do we have some file/reference i should upload for a fate test?

thx

[...]
Ronald S. Bultje June 19, 2017, 6:44 p.m. UTC | #2 
Hi,

On Fri, Jun 16, 2017 at 12:46 PM, Michael Niedermayer <
michael@niedermayer.cc> wrote:

> On Thu, Jun 15, 2017 at 03:50:31PM -0400, Ronald S. Bultje wrote:
> > From: Anton Mitrofanov <BugMaster@narod.ru>
> >
> > Use the correct ctxIdxInc calculation for coded_block_flag.
> > Keep old behavior for old versions of x264 for backward compatibility.
> >
> > Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
> > ---
> >  libavcodec/h264_cabac.c | 47 ++++++++++++++++++++++++++++++
> +++--------------
> >  1 file changed, 33 insertions(+), 14 deletions(-)
>
> patchset should be ok if tested against some reference
>

Pushed.

also do we have some file/reference i should upload for a fate test?


(Anton provided those on IRC.)

Ronald
Michael Niedermayer June 19, 2017, 7:55 p.m. UTC | #3 
On Mon, Jun 19, 2017 at 02:44:47PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Jun 16, 2017 at 12:46 PM, Michael Niedermayer <
> michael@niedermayer.cc> wrote:
> 
> > On Thu, Jun 15, 2017 at 03:50:31PM -0400, Ronald S. Bultje wrote:
> > > From: Anton Mitrofanov <BugMaster@narod.ru>
> > >
> > > Use the correct ctxIdxInc calculation for coded_block_flag.
> > > Keep old behavior for old versions of x264 for backward compatibility.
> > >
> > > Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
> > > ---
> > >  libavcodec/h264_cabac.c | 47 ++++++++++++++++++++++++++++++
> > +++--------------
> > >  1 file changed, 33 insertions(+), 14 deletions(-)
> >
> > patchset should be ok if tested against some reference
> >
> 
> Pushed.
> 
> also do we have some file/reference i should upload for a fate test?
> 
> 
> (Anton provided those on IRC.)

ive uploaded the 2 files when they were mentioned on IRC

h264-444/i444_hybrid_+i8x8_+pcm.264  and h264-444/old_i444_lossless_+i8x8_+pcm.264

[...]
wm4 Dec. 23, 2017, 12:27 a.m. UTC | #4 
On Thu, 15 Jun 2017 15:50:31 -0400
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> From: Anton Mitrofanov <BugMaster@narod.ru>
> 
> Use the correct ctxIdxInc calculation for coded_block_flag.
> Keep old behavior for old versions of x264 for backward compatibility.
> 
> Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
> ---
>  libavcodec/h264_cabac.c | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/libavcodec/h264_cabac.c b/libavcodec/h264_cabac.c
> index 11ff3a0..28aacc5 100644
> --- a/libavcodec/h264_cabac.c
> +++ b/libavcodec/h264_cabac.c
> @@ -2347,21 +2347,40 @@ decode_intra_mb:
>      if (CHROMA444(h) && IS_8x8DCT(mb_type)){
>          int i;
>          uint8_t *nnz_cache = sl->non_zero_count_cache;
> -        for (i = 0; i < 2; i++){
> -            if (sl->left_type[LEFT(i)] && !IS_8x8DCT(sl->left_type[LEFT(i)])) {
> -                nnz_cache[3+8* 1 + 2*8*i]=
> -                nnz_cache[3+8* 2 + 2*8*i]=
> -                nnz_cache[3+8* 6 + 2*8*i]=
> -                nnz_cache[3+8* 7 + 2*8*i]=
> -                nnz_cache[3+8*11 + 2*8*i]=
> -                nnz_cache[3+8*12 + 2*8*i]= IS_INTRA(mb_type) ? 64 : 0;
> +        if (h->sei.unregistered.x264_build < 151U) {
> +            for (i = 0; i < 2; i++){
> +                if (sl->left_type[LEFT(i)] && !IS_8x8DCT(sl->left_type[LEFT(i)])) {
> +                    nnz_cache[3+8* 1 + 2*8*i]=
> +                    nnz_cache[3+8* 2 + 2*8*i]=
> +                    nnz_cache[3+8* 6 + 2*8*i]=
> +                    nnz_cache[3+8* 7 + 2*8*i]=
> +                    nnz_cache[3+8*11 + 2*8*i]=
> +                    nnz_cache[3+8*12 + 2*8*i]= IS_INTRA(mb_type) ? 64 : 0;
> +                }
> +            }
> +            if (sl->top_type && !IS_8x8DCT(sl->top_type)){
> +                uint32_t top_empty = !IS_INTRA(mb_type) ? 0 : 0x40404040;
> +                AV_WN32A(&nnz_cache[4+8* 0], top_empty);
> +                AV_WN32A(&nnz_cache[4+8* 5], top_empty);
> +                AV_WN32A(&nnz_cache[4+8*10], top_empty);
> +            }
> +        } else {
> +            for (i = 0; i < 2; i++){
> +                if (sl->left_type[LEFT(i)] && !IS_8x8DCT(sl->left_type[LEFT(i)])) {
> +                    nnz_cache[3+8* 1 + 2*8*i]=
> +                    nnz_cache[3+8* 2 + 2*8*i]=
> +                    nnz_cache[3+8* 6 + 2*8*i]=
> +                    nnz_cache[3+8* 7 + 2*8*i]=
> +                    nnz_cache[3+8*11 + 2*8*i]=
> +                    nnz_cache[3+8*12 + 2*8*i]= !IS_INTRA_PCM(sl->left_type[LEFT(i)]) ? 0 : 64;
> +                }
> +            }
> +            if (sl->top_type && !IS_8x8DCT(sl->top_type)){
> +                uint32_t top_empty = !IS_INTRA_PCM(sl->top_type) ? 0 : 0x40404040;
> +                AV_WN32A(&nnz_cache[4+8* 0], top_empty);
> +                AV_WN32A(&nnz_cache[4+8* 5], top_empty);
> +                AV_WN32A(&nnz_cache[4+8*10], top_empty);
>              }
> -        }
> -        if (sl->top_type && !IS_8x8DCT(sl->top_type)){
> -            uint32_t top_empty = !IS_INTRA(mb_type) ? 0 : 0x40404040;
> -            AV_WN32A(&nnz_cache[4+8* 0], top_empty);
> -            AV_WN32A(&nnz_cache[4+8* 5], top_empty);
> -            AV_WN32A(&nnz_cache[4+8*10], top_empty);
>          }
>      }
>      h->cur_pic.mb_type[mb_xy] = mb_type;

There have been several user complaints about it. Apparently, there are
a lot of these broken files around. The problem is that some files lack
the SEI (or it has been wiped out by incompetents to hide encode
settings, I guess). Usually, only the first video packet in a file
contains this SEI, so starting at a position other than the start also
causes problems.

Would there be any way to detect this situation if x264_build is unset?
Such as retrying decoding after obvious errors happen, or at least
allowing frames after that to be decoded with the bug workaround
enabled. Or would that be too crude to be considered?
diff mbox

Patch

diff --git a/libavcodec/h264_cabac.c b/libavcodec/h264_cabac.c
index 11ff3a0..28aacc5 100644
--- a/libavcodec/h264_cabac.c
+++ b/libavcodec/h264_cabac.c
@@ -2347,21 +2347,40 @@  decode_intra_mb:
     if (CHROMA444(h) && IS_8x8DCT(mb_type)){
         int i;
         uint8_t *nnz_cache = sl->non_zero_count_cache;
-        for (i = 0; i < 2; i++){
-            if (sl->left_type[LEFT(i)] && !IS_8x8DCT(sl->left_type[LEFT(i)])) {
-                nnz_cache[3+8* 1 + 2*8*i]=
-                nnz_cache[3+8* 2 + 2*8*i]=
-                nnz_cache[3+8* 6 + 2*8*i]=
-                nnz_cache[3+8* 7 + 2*8*i]=
-                nnz_cache[3+8*11 + 2*8*i]=
-                nnz_cache[3+8*12 + 2*8*i]= IS_INTRA(mb_type) ? 64 : 0;
+        if (h->sei.unregistered.x264_build < 151U) {
+            for (i = 0; i < 2; i++){
+                if (sl->left_type[LEFT(i)] && !IS_8x8DCT(sl->left_type[LEFT(i)])) {
+                    nnz_cache[3+8* 1 + 2*8*i]=
+                    nnz_cache[3+8* 2 + 2*8*i]=
+                    nnz_cache[3+8* 6 + 2*8*i]=
+                    nnz_cache[3+8* 7 + 2*8*i]=
+                    nnz_cache[3+8*11 + 2*8*i]=
+                    nnz_cache[3+8*12 + 2*8*i]= IS_INTRA(mb_type) ? 64 : 0;
+                }
+            }
+            if (sl->top_type && !IS_8x8DCT(sl->top_type)){
+                uint32_t top_empty = !IS_INTRA(mb_type) ? 0 : 0x40404040;
+                AV_WN32A(&nnz_cache[4+8* 0], top_empty);
+                AV_WN32A(&nnz_cache[4+8* 5], top_empty);
+                AV_WN32A(&nnz_cache[4+8*10], top_empty);
+            }
+        } else {
+            for (i = 0; i < 2; i++){
+                if (sl->left_type[LEFT(i)] && !IS_8x8DCT(sl->left_type[LEFT(i)])) {
+                    nnz_cache[3+8* 1 + 2*8*i]=
+                    nnz_cache[3+8* 2 + 2*8*i]=
+                    nnz_cache[3+8* 6 + 2*8*i]=
+                    nnz_cache[3+8* 7 + 2*8*i]=
+                    nnz_cache[3+8*11 + 2*8*i]=
+                    nnz_cache[3+8*12 + 2*8*i]= !IS_INTRA_PCM(sl->left_type[LEFT(i)]) ? 0 : 64;
+                }
+            }
+            if (sl->top_type && !IS_8x8DCT(sl->top_type)){
+                uint32_t top_empty = !IS_INTRA_PCM(sl->top_type) ? 0 : 0x40404040;
+                AV_WN32A(&nnz_cache[4+8* 0], top_empty);
+                AV_WN32A(&nnz_cache[4+8* 5], top_empty);
+                AV_WN32A(&nnz_cache[4+8*10], top_empty);
             }
-        }
-        if (sl->top_type && !IS_8x8DCT(sl->top_type)){
-            uint32_t top_empty = !IS_INTRA(mb_type) ? 0 : 0x40404040;
-            AV_WN32A(&nnz_cache[4+8* 0], top_empty);
-            AV_WN32A(&nnz_cache[4+8* 5], top_empty);
-            AV_WN32A(&nnz_cache[4+8*10], top_empty);
         }
     }
     h->cur_pic.mb_type[mb_xy] = mb_type;