| Message ID | 20220403214337.4090-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/ituh263enc: Add AV_CODEC_CAP_SLICE_THREADS to old H.263 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Michael Niedermayer: > It is supported by the H.263+ AVCodec already > > Is there any case where this does not work ? > > Fixes regression of some command lines > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/ituh263enc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/ituh263enc.c b/libavcodec/ituh263enc.c > index db7cdf1fcb..82dce05e36 100644 > --- a/libavcodec/ituh263enc.c > +++ b/libavcodec/ituh263enc.c > @@ -908,6 +908,7 @@ const FFCodec ff_h263_encoder = { > .p.id = AV_CODEC_ID_H263, > .p.pix_fmts = (const enum AVPixelFormat[]){AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE}, > .p.priv_class = &h263_class, > + .p.capabilities = AV_CODEC_CAP_SLICE_THREADS, > .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, > .priv_data_size = sizeof(MpegEncContext), > .init = ff_mpv_encode_init, 1. If you claim that there is a regression, you should mention the commit that introduced them in the commit message (it's obviously 8ca4b515e73079cda068e253853654db394b8171 in this case). 2. What command lines regressed exactly? The only command lines that should be affected by said commit are command lines that set the slices option to a value > 1. 3. As the commit message of 8ca4b515e73079cda068e253853654db394b8171 explains, this was intentional, as the H.263 encoder produces broken files with multiple slices (whether with slice-threading or not). One gets all kinds of error messages when decoding such a file: "I cbpy damaged at 1 7", "Error at MB: 316", "illegal ac vlc code at 0x29", "slice end not reached but screenspace end (7 left 800000, score= -125)", "run overflow at 0x7 i:1". Of course, there are visual artifacts, too. 4. With this patch, this encoder will by default (at least, by the defaults of the ffmpeg command line tool) produce broken files. 5. "Is there any case where this does not work ?": Is there any where it works? - Andreas
On Tue, Apr 05, 2022 at 05:07:22PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > It is supported by the H.263+ AVCodec already > > > > Is there any case where this does not work ? > > > > Fixes regression of some command lines > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/ituh263enc.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/ituh263enc.c b/libavcodec/ituh263enc.c > > index db7cdf1fcb..82dce05e36 100644 > > --- a/libavcodec/ituh263enc.c > > +++ b/libavcodec/ituh263enc.c > > @@ -908,6 +908,7 @@ const FFCodec ff_h263_encoder = { > > .p.id = AV_CODEC_ID_H263, > > .p.pix_fmts = (const enum AVPixelFormat[]){AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE}, > > .p.priv_class = &h263_class, > > + .p.capabilities = AV_CODEC_CAP_SLICE_THREADS, > > .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, > > .priv_data_size = sizeof(MpegEncContext), > > .init = ff_mpv_encode_init, > > 1. If you claim that there is a regression, you should mention the > commit that introduced them in the commit message (it's obviously > 8ca4b515e73079cda068e253853654db394b8171 in this case). > 2. What command lines regressed exactly? The only command lines that > should be affected by said commit are command lines that set the slices > option to a value > 1. > 3. As the commit message of 8ca4b515e73079cda068e253853654db394b8171 > explains, this was intentional, as the H.263 encoder produces broken > files with multiple slices (whether with slice-threading or not). One > gets all kinds of error messages when decoding such a file: "I cbpy > damaged at 1 7", "Error at MB: 316", "illegal ac vlc code at 0x29", > "slice end not reached but screenspace end (7 left 800000, score= > -125)", "run overflow at 0x7 i:1". Of course, there are visual > artifacts, too. > 4. With this patch, this encoder will by default (at least, by the > defaults of the ffmpeg command line tool) produce broken files. > 5. "Is there any case where this does not work ?": Is there any where it > works? The testcases i had where these: ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 1 -y -threads 1 -vcodec h263 -s 352x288 -an /tmp/file-h263-s1t1.h263 ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 2 -y -threads 1 -vcodec h263 -s 352x288 -an /tmp/file-h263-s2t1.h263 ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 1 -y -threads 2 -vcodec h263 -s 352x288 -an /tmp/file-h263-s1t2.h263 ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 2 -y -threads 2 -vcodec h263 -s 352x288 -an /tmp/file-h263-s2t2.h263 The files seem to play fine i did not try to find a case that fails thx [...]
Michael Niedermayer: > On Tue, Apr 05, 2022 at 05:07:22PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> It is supported by the H.263+ AVCodec already >>> >>> Is there any case where this does not work ? >>> >>> Fixes regression of some command lines >>> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/ituh263enc.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/libavcodec/ituh263enc.c b/libavcodec/ituh263enc.c >>> index db7cdf1fcb..82dce05e36 100644 >>> --- a/libavcodec/ituh263enc.c >>> +++ b/libavcodec/ituh263enc.c >>> @@ -908,6 +908,7 @@ const FFCodec ff_h263_encoder = { >>> .p.id = AV_CODEC_ID_H263, >>> .p.pix_fmts = (const enum AVPixelFormat[]){AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE}, >>> .p.priv_class = &h263_class, >>> + .p.capabilities = AV_CODEC_CAP_SLICE_THREADS, >>> .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, >>> .priv_data_size = sizeof(MpegEncContext), >>> .init = ff_mpv_encode_init, >> >> 1. If you claim that there is a regression, you should mention the >> commit that introduced them in the commit message (it's obviously >> 8ca4b515e73079cda068e253853654db394b8171 in this case). >> 2. What command lines regressed exactly? The only command lines that >> should be affected by said commit are command lines that set the slices >> option to a value > 1. >> 3. As the commit message of 8ca4b515e73079cda068e253853654db394b8171 >> explains, this was intentional, as the H.263 encoder produces broken >> files with multiple slices (whether with slice-threading or not). One >> gets all kinds of error messages when decoding such a file: "I cbpy >> damaged at 1 7", "Error at MB: 316", "illegal ac vlc code at 0x29", >> "slice end not reached but screenspace end (7 left 800000, score= >> -125)", "run overflow at 0x7 i:1". Of course, there are visual >> artifacts, too. >> 4. With this patch, this encoder will by default (at least, by the >> defaults of the ffmpeg command line tool) produce broken files. >> 5. "Is there any case where this does not work ?": Is there any where it >> works? > > The testcases i had where these: > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 1 -y -threads 1 -vcodec h263 -s 352x288 -an /tmp/file-h263-s1t1.h263 > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 2 -y -threads 1 -vcodec h263 -s 352x288 -an /tmp/file-h263-s2t1.h263 > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 1 -y -threads 2 -vcodec h263 -s 352x288 -an /tmp/file-h263-s1t2.h263 > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 2 -y -threads 2 -vcodec h263 -s 352x288 -an /tmp/file-h263-s2t2.h263 > > The files seem to play fine > i did not try to find a case that fails > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 4 -y -threads 4 -vcodec h263 -s 1408x1152 -an /tmp/file-h263-s2t2.h263 produces garbage; also with -s 704x576. slices < 4 seem fine. If one uses too many slices with smaller resolutions, the file is no longer correctly probed, but can be correctly decoded with -f h263. I don't know what is wrong with the bigger resolutions and too many slices; I don't know H.263 at all. My first (and admittedly only) test for whether using multiple slices with a single thread works produced garbage, so I put this codec in the "multiple slices not supported" box. - Andreas
On Wed, Apr 06, 2022 at 03:05:07AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Tue, Apr 05, 2022 at 05:07:22PM +0200, Andreas Rheinhardt wrote: > >> Michael Niedermayer: > >>> It is supported by the H.263+ AVCodec already > >>> > >>> Is there any case where this does not work ? > >>> > >>> Fixes regression of some command lines > >>> > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavcodec/ituh263enc.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/libavcodec/ituh263enc.c b/libavcodec/ituh263enc.c > >>> index db7cdf1fcb..82dce05e36 100644 > >>> --- a/libavcodec/ituh263enc.c > >>> +++ b/libavcodec/ituh263enc.c > >>> @@ -908,6 +908,7 @@ const FFCodec ff_h263_encoder = { > >>> .p.id = AV_CODEC_ID_H263, > >>> .p.pix_fmts = (const enum AVPixelFormat[]){AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE}, > >>> .p.priv_class = &h263_class, > >>> + .p.capabilities = AV_CODEC_CAP_SLICE_THREADS, > >>> .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, > >>> .priv_data_size = sizeof(MpegEncContext), > >>> .init = ff_mpv_encode_init, > >> > >> 1. If you claim that there is a regression, you should mention the > >> commit that introduced them in the commit message (it's obviously > >> 8ca4b515e73079cda068e253853654db394b8171 in this case). > >> 2. What command lines regressed exactly? The only command lines that > >> should be affected by said commit are command lines that set the slices > >> option to a value > 1. > >> 3. As the commit message of 8ca4b515e73079cda068e253853654db394b8171 > >> explains, this was intentional, as the H.263 encoder produces broken > >> files with multiple slices (whether with slice-threading or not). One > >> gets all kinds of error messages when decoding such a file: "I cbpy > >> damaged at 1 7", "Error at MB: 316", "illegal ac vlc code at 0x29", > >> "slice end not reached but screenspace end (7 left 800000, score= > >> -125)", "run overflow at 0x7 i:1". Of course, there are visual > >> artifacts, too. > >> 4. With this patch, this encoder will by default (at least, by the > >> defaults of the ffmpeg command line tool) produce broken files. > >> 5. "Is there any case where this does not work ?": Is there any where it > >> works? > > > > The testcases i had where these: > > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 1 -y -threads 1 -vcodec h263 -s 352x288 -an /tmp/file-h263-s1t1.h263 > > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 2 -y -threads 1 -vcodec h263 -s 352x288 -an /tmp/file-h263-s2t1.h263 > > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 1 -y -threads 2 -vcodec h263 -s 352x288 -an /tmp/file-h263-s1t2.h263 > > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t 1 -bitexact -qscale 2 -slices 2 -y -threads 2 -vcodec h263 -s 352x288 -an /tmp/file-h263-s2t2.h263 > > > > The files seem to play fine > > i did not try to find a case that fails > > > > ./ffmpeg -threads 1 -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -t > 1 -bitexact -qscale 2 -slices 4 -y -threads 4 -vcodec h263 -s 1408x1152 > -an /tmp/file-h263-s2t2.h263 > > produces garbage; also with -s 704x576. slices < 4 seem fine. If one > uses too many slices with smaller resolutions, the file is no longer > correctly probed, but can be correctly decoded with -f h263. > I don't know what is wrong with the bigger resolutions and too many > slices; I don't know H.263 at all. My first (and admittedly only) test > for whether using multiple slices with a single thread works produced > garbage, so I put this codec in the "multiple slices not supported" box. Its a while ago but H263+ has a nice slice structired mode H263 lacks that and uses a more restricted mode see ff_h263_encode_gob_header() I would wildly guess these restrictions are what causes some cases not to work but i didnt check thx [...]
diff --git a/libavcodec/ituh263enc.c b/libavcodec/ituh263enc.c index db7cdf1fcb..82dce05e36 100644 --- a/libavcodec/ituh263enc.c +++ b/libavcodec/ituh263enc.c @@ -908,6 +908,7 @@ const FFCodec ff_h263_encoder = { .p.id = AV_CODEC_ID_H263, .p.pix_fmts = (const enum AVPixelFormat[]){AV_PIX_FMT_YUV420P, AV_PIX_FMT_NONE}, .p.priv_class = &h263_class, + .p.capabilities = AV_CODEC_CAP_SLICE_THREADS, .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, .priv_data_size = sizeof(MpegEncContext), .init = ff_mpv_encode_init,
It is supported by the H.263+ AVCodec already Is there any case where this does not work ? Fixes regression of some command lines Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ituh263enc.c | 1 + 1 file changed, 1 insertion(+)