diff mbox

[FFmpeg-devel] lavc: Allow forcing work-around for x264 cabac 8x8 4:4:4 bug

Message ID CAB0OVGpQdn1G8=VNju6_WbMw65gLc3VOApK=5gKP9BgCN0=9Hw@mail.gmail.com
State Superseded
Headers show

Commit Message

Carl Eugen Hoyos Dec. 4, 2017, 12:09 a.m. UTC
Hi!

Attached patch fixes ticket #6717, files without sei can be produced
with remuxing and seeking, even if this is a (separate) bug, such
files exist in the wild.

Please comment, Carl Eugen

Comments

Mark Thompson Dec. 4, 2017, 12:56 a.m. UTC | #1
On 04/12/17 00:09, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch fixes ticket #6717, files without sei can be produced
> with remuxing and seeking, even if this is a (separate) bug, such
> files exist in the wild.
> 
> Please comment, Carl Eugen

File are currently fixable by remuxing with:

-bsf:v 'h264_metadata=sei_user_data=dc45e9bde6d948b7962cd820d923eeef+x264 - core 150'

Unfortunately that can't be applied directly when decoding, because -bsf isn't available as an input option.

I don't really like the idea of adding the option because it does break correct files, but I guess it probably ends up being the pragmatic option for decoding.

Still, I think it might be sensible to at least add a big warning telling the user they're doing something stupid if this option is set and it finds x264_build is >= 151?

- Mark
Carl Eugen Hoyos Dec. 4, 2017, 2:21 a.m. UTC | #2
2017-12-04 1:56 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> On 04/12/17 00:09, Carl Eugen Hoyos wrote:

>> Attached patch fixes ticket #6717, files without sei can be produced
>> with remuxing and seeking, even if this is a (separate) bug, such
>> files exist in the wild.

> File are currently fixable by remuxing with:
>
> -bsf:v 'h264_metadata=sei_user_data=dc45e9bde6d948b7962cd820d923eeef+x264 - core 150'

Thank you!

> Unfortunately that can't be applied directly when decoding,
> because -bsf isn't available as an input option.

> I don't really like the idea of adding the option because it does
> break correct files, but I guess it probably ends up being the
> pragmatic option for decoding.

Sorry, I don't understand: Do you mean if the user specifies
the work-around, it will break files encoded with current
x264? Isn't that the whole point of this (and every other)
work-around?

> Still, I think it might be sensible to at least add a big warning
> telling the user they're doing something stupid if this option
> is set and it finds x264_build is >= 151?

I believe you know that I am a big fan of warnings but printing
a warning when a user explicitely specifies an option? We
don't warn when an experimental encoder is used (not even
when we know it will segfault) and I don't think it is necessary.

Thank you, Carl Eugen
wm4 Dec. 4, 2017, 9:45 p.m. UTC | #3
On Mon, 4 Dec 2017 00:56:36 +0000
Mark Thompson <sw@jkqxz.net> wrote:

> On 04/12/17 00:09, Carl Eugen Hoyos wrote:
> > Hi!
> > 
> > Attached patch fixes ticket #6717, files without sei can be produced
> > with remuxing and seeking, even if this is a (separate) bug, such
> > files exist in the wild.
> > 
> > Please comment, Carl Eugen  
> 
> File are currently fixable by remuxing with:
> 
> -bsf:v 'h264_metadata=sei_user_data=dc45e9bde6d948b7962cd820d923eeef+x264 - core 150'

Is that only in the first packet? Then it doesn't work if you start
playback at a later frame, unless you somehow decode the first packet
anyway.

IMO libavcodec should detect buggy files without relying on version
info.
Mark Thompson Dec. 4, 2017, 11:44 p.m. UTC | #4
On 04/12/17 02:21, Carl Eugen Hoyos wrote:
> 2017-12-04 1:56 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>> On 04/12/17 00:09, Carl Eugen Hoyos wrote:
> 
>>> Attached patch fixes ticket #6717, files without sei can be produced
>>> with remuxing and seeking, even if this is a (separate) bug, such
>>> files exist in the wild.
> 
>> File are currently fixable by remuxing with:
>>
>> -bsf:v 'h264_metadata=sei_user_data=dc45e9bde6d948b7962cd820d923eeef+x264 - core 150'
> 
> Thank you!
> 
>> Unfortunately that can't be applied directly when decoding,
>> because -bsf isn't available as an input option.
> 
>> I don't really like the idea of adding the option because it does
>> break correct files, but I guess it probably ends up being the
>> pragmatic option for decoding.
> 
> Sorry, I don't understand: Do you mean if the user specifies
> the work-around, it will break files encoded with current
> x264? Isn't that the whole point of this (and every other)
> work-around?
> 
>> Still, I think it might be sensible to at least add a big warning
>> telling the user they're doing something stupid if this option
>> is set and it finds x264_build is >= 151?
> 
> I believe you know that I am a big fan of warnings but printing
> a warning when a user explicitely specifies an option? We
> don't warn when an experimental encoder is used (not even
> when we know it will segfault) and I don't think it is necessary.

There are three cases here:

i)   The file contains the SEI for x264 build < 151.
ii)  The file contains the SEI for x264 build >= 151.
iii) The file does not contain any x264 SEI, so we can't tell whether it is broken.

I'm suggesting printing a warning only in case (ii), where we know that applying the workaround is almost certainly wrong but will of course do so anyway.

(Feel free to ignore this suggestion.)

- Mark
Mark Thompson Dec. 4, 2017, 11:54 p.m. UTC | #5
On 04/12/17 21:45, wm4 wrote:
> On Mon, 4 Dec 2017 00:56:36 +0000
> Mark Thompson <sw@jkqxz.net> wrote:
> 
>> On 04/12/17 00:09, Carl Eugen Hoyos wrote:
>>> Hi!
>>>
>>> Attached patch fixes ticket #6717, files without sei can be produced
>>> with remuxing and seeking, even if this is a (separate) bug, such
>>> files exist in the wild.
>>>
>>> Please comment, Carl Eugen  
>>
>> File are currently fixable by remuxing with:
>>
>> -bsf:v 'h264_metadata=sei_user_data=dc45e9bde6d948b7962cd820d923eeef+x264 - core 150'
> 
> Is that only in the first packet? Then it doesn't work if you start
> playback at a later frame, unless you somehow decode the first packet
> anyway.

It adds it to every access unit containing an SPS, which will cover this in most cases.  (Though not all, I admit - SEI not being allowed in AVCC extradata means that if the parameter sets are only there then you are stuck.)

> IMO libavcodec should detect buggy files without relying on version
> info.
Unfortunately that is rather difficult in this case - pretty much the only method to have it happen automatically would be to try decoding the whole frame again in the other mode if there is a failure during entropy decoding, which doesn't sound very fun to write.

- Mark
diff mbox

Patch

From c3ce0a75f13663958034a70fb4a982671532d269 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Mon, 4 Dec 2017 01:05:57 +0100
Subject: [PATCH] lavc: Allow forcing work-around for old x264 cabac 8x8 4:4:4
 bug.

Fixes ticket #6717.
---
 libavcodec/avcodec.h       |    1 +
 libavcodec/h264_cabac.c    |    5 ++++-
 libavcodec/options_table.h |    1 +
 libavcodec/version.h       |    2 +-
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 5db6a81..bae8812 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2516,6 +2516,7 @@  typedef struct AVCodecContext {
      */
     int workaround_bugs;
 #define FF_BUG_AUTODETECT       1  ///< autodetection
+#define FF_BUG_X264_CAB_8x8_444 2
 #define FF_BUG_XVID_ILACE       4
 #define FF_BUG_UMP4             8
 #define FF_BUG_NO_PADDING       16
diff --git a/libavcodec/h264_cabac.c b/libavcodec/h264_cabac.c
index ec5fc74..8b2973a 100644
--- a/libavcodec/h264_cabac.c
+++ b/libavcodec/h264_cabac.c
@@ -1919,6 +1919,9 @@  int ff_h264_decode_mb_cabac(const H264Context *h, H264SliceContext *sl)
     int dct8x8_allowed = h->ps.pps->transform_8x8_mode;
     const int decode_chroma = sps->chroma_format_idc == 1 || sps->chroma_format_idc == 2;
     const int pixel_shift = h->pixel_shift;
+    AVCodecContext *avctx = h->avctx;
+    if (h->x264_build < 151U)
+        avctx->workaround_bugs |= FF_BUG_X264_CAB_8x8_444;
 
     mb_xy = sl->mb_xy = sl->mb_x + sl->mb_y*h->mb_stride;
 
@@ -2347,7 +2350,7 @@  decode_intra_mb:
     if (CHROMA444(h) && IS_8x8DCT(mb_type)){
         int i;
         uint8_t *nnz_cache = sl->non_zero_count_cache;
-        if (h->x264_build < 151U) {
+        if (avctx->workaround_bugs & FF_BUG_X264_CAB_8x8_444) {
             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]=
diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index d89f58d..ed35bec 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -112,6 +112,7 @@  static const AVOption avcodec_options[] = {
 {"bug", "work around not autodetected encoder bugs", OFFSET(workaround_bugs), AV_OPT_TYPE_FLAGS, {.i64 = FF_BUG_AUTODETECT }, INT_MIN, INT_MAX, V|D, "bug"},
 {"autodetect", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_BUG_AUTODETECT }, INT_MIN, INT_MAX, V|D, "bug"},
 {"xvid_ilace", "Xvid interlacing bug (autodetected if FOURCC == XVIX)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_BUG_XVID_ILACE }, INT_MIN, INT_MAX, V|D, "bug"},
+{"x264_cab_8x8_444", "x264 cabac 8x8 4:4:4 encoding bug (autodetected if possible)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_BUG_X264_CAB_8x8_444 }, INT_MIN, INT_MAX, V|D, "bug"},
 {"ump4", "(autodetected if FOURCC == UMP4)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_BUG_UMP4 }, INT_MIN, INT_MAX, V|D, "bug"},
 {"no_padding", "padding bug (autodetected)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_BUG_NO_PADDING }, INT_MIN, INT_MAX, V|D, "bug"},
 {"amv", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_BUG_AMV }, INT_MIN, INT_MAX, V|D, "bug"},
diff --git a/libavcodec/version.h b/libavcodec/version.h
index d67b689..3b5c300 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@ 
 
 #define LIBAVCODEC_VERSION_MAJOR  58
 #define LIBAVCODEC_VERSION_MINOR   6
-#define LIBAVCODEC_VERSION_MICRO 102
+#define LIBAVCODEC_VERSION_MICRO 103
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \
-- 
1.7.10.4