[PATCH 1/3] avcodec/ffv1enc: Slice combination is unsupported
We always write minimal slices, the size calculation is wrong in some corner cases but as its always 1x1 (minus1) we can for now just hard-code it This helps with ticket 5548 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ffv1enc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 2778c63012e..4d0ddc167ae 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -914,8 +914,8 @@ static void encode_slice_header(FFV1Context *f, FFV1Context *fs) put_symbol(c, state, (fs->slice_x +1)*f->num_h_slices / f->width , 0); put_symbol(c, state, (fs->slice_y +1)*f->num_v_slices / f->height , 0); - put_symbol(c, state, (fs->slice_width +1)*f->num_h_slices / f->width -1, 0); - put_symbol(c, state, (fs->slice_height+1)*f->num_v_slices / f->height-1, 0); + put_symbol(c, state, 0, 0); + put_symbol(c, state, 0, 0); for (j=0; j<f->plane_count; j++) { put_symbol(c, state, f->plane[j].quant_table_index, 0); av_assert0(f->plane[j].quant_table_index == f->context_model); -- 2.17.1
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ffv1.c | 2 ++ libavcodec/ffv1.h | 1 + libavcodec/ffv1enc.c | 4 ++-- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/libavcodec/ffv1.c b/libavcodec/ffv1.c index b6204740edb..f44fead350b 100644 --- a/libavcodec/ffv1.c +++ b/libavcodec/ffv1.c @@ -129,6 +129,8 @@ av_cold int ff_ffv1_init_slice_contexts(FFV1Context *f) fs->slice_height = sye - sys; fs->slice_x = sxs; fs->slice_y = sys; + fs->sx = sx; + fs->sy = sy; fs->sample_buffer = av_malloc_array((fs->width + 6), 3 * MAX_PLANES * sizeof(*fs->sample_buffer)); diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h index 04869da5c9a..14e40936065 100644 --- a/libavcodec/ffv1.h +++ b/libavcodec/ffv1.h @@ -129,6 +129,7 @@ typedef struct FFV1Context { int slice_height; int slice_x; int slice_y; + int sx, sy; int slice_reset_contexts; int slice_coding_mode; int slice_rct_by_coef; diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 4d0ddc167ae..11be7fe32ea 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -912,8 +912,8 @@ static void encode_slice_header(FFV1Context *f, FFV1Context *fs) int j; memset(state, 128, sizeof(state)); - put_symbol(c, state, (fs->slice_x +1)*f->num_h_slices / f->width , 0); - put_symbol(c, state, (fs->slice_y +1)*f->num_v_slices / f->height , 0); + put_symbol(c, state, fs->sx, 0); + put_symbol(c, state, fs->sy, 0); put_symbol(c, state, 0, 0); put_symbol(c, state, 0, 0); for (j=0; j<f->plane_count; j++) { -- 2.17.1
This fixes green lines in some odd dimensions with some slice configurations like Ticket 5548 This also changes the encoder and whats encoded, and would require an update to the specification. This change attempts to limit the change to configurations that have missing lines currently. Testing is welcome and needed Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ffv1.c | 38 ++++++++++++++++++++++++++++++++++---- libavcodec/ffv1.h | 2 ++ libavcodec/ffv1dec.c | 8 ++++---- libavcodec/ffv1enc.c | 7 ++++--- 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/libavcodec/ffv1.c b/libavcodec/ffv1.c index f44fead350b..e780c910aee 100644 --- a/libavcodec/ffv1.c +++ b/libavcodec/ffv1.c @@ -103,6 +103,36 @@ av_cold int ff_ffv1_init_slices_state(FFV1Context *f) return 0; } +int ff_need_new_slices(int width, int num_h_slices, int chroma_shift) { + int mpw = 1<<chroma_shift; + int i = width * (int64_t)(num_h_slices - 1) / num_h_slices; + + return width % mpw && (width - i) % mpw == 0; +} + +int ff_slice_coord(const FFV1Context *f, int width, int sx, int num_h_slices, int chroma_shift) { + int mpw = 1<<chroma_shift; + int awidth = FFALIGN(width, mpw); + int old = 1; + + if (f->version > 4) { + old = 0; + } else if (f->version == 3 && f->micro_version > 4 || + f->version == 4 && f->micro_version > 2 + ) { + // If the old system will not encode all pixels we will choose the new + old = !ff_need_new_slices(width, num_h_slices, chroma_shift); + } + + if (old) + return width * sx / num_h_slices; + + sx = (awidth * (int64_t)sx + (num_h_slices * mpw / 2)) / (num_h_slices * mpw) * mpw; + if (sx == awidth) + sx = width; + return sx; +} + av_cold int ff_ffv1_init_slice_contexts(FFV1Context *f) { int i, max_slice_count = f->num_h_slices * f->num_v_slices; @@ -112,10 +142,10 @@ av_cold int ff_ffv1_init_slice_contexts(FFV1Context *f) for (i = 0; i < max_slice_count;) { int sx = i % f->num_h_slices; int sy = i / f->num_h_slices; - int sxs = f->avctx->width * sx / f->num_h_slices; - int sxe = f->avctx->width * (sx + 1) / f->num_h_slices; - int sys = f->avctx->height * sy / f->num_v_slices; - int sye = f->avctx->height * (sy + 1) / f->num_v_slices; + int sxs = ff_slice_coord(f, f->avctx->width , sx , f->num_h_slices, f->chroma_h_shift); + int sxe = ff_slice_coord(f, f->avctx->width , sx + 1, f->num_h_slices, f->chroma_h_shift); + int sys = ff_slice_coord(f, f->avctx->height, sy , f->num_v_slices, f->chroma_v_shift); + int sye = ff_slice_coord(f, f->avctx->height, sy + 1, f->num_v_slices, f->chroma_v_shift); FFV1Context *fs = av_mallocz(sizeof(*fs)); if (!fs) diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h index 14e40936065..3f658a26522 100644 --- a/libavcodec/ffv1.h +++ b/libavcodec/ffv1.h @@ -143,6 +143,8 @@ int ff_ffv1_init_slice_contexts(FFV1Context *f); int ff_ffv1_allocate_initial_states(FFV1Context *f); void ff_ffv1_clear_slice_state(const FFV1Context *f, FFV1Context *fs); int ff_ffv1_close(AVCodecContext *avctx); +int ff_need_new_slices(int width, int num_h_slices, int chroma_shift); +int ff_slice_coord(const FFV1Context *f, int width, int sx, int num_h_slices, int chroma_shift); static av_always_inline int fold(int diff, int bits) { diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index a376a4c8170..0486938eb2b 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -184,10 +184,10 @@ static int decode_slice_header(const FFV1Context *f, FFV1Context *fs) if (sx > f->num_h_slices - sw || sy > f->num_v_slices - sh) return AVERROR_INVALIDDATA; - fs->slice_x = sx * (int64_t)f->width / f->num_h_slices; - fs->slice_y = sy * (int64_t)f->height / f->num_v_slices; - fs->slice_width = (sx + sw) * (int64_t)f->width / f->num_h_slices - fs->slice_x; - fs->slice_height = (sy + sh) * (int64_t)f->height / f->num_v_slices - fs->slice_y; + fs->slice_x = ff_slice_coord(f, f->width , sx , f->num_h_slices, f->chroma_h_shift); + fs->slice_y = ff_slice_coord(f, f->height, sy , f->num_v_slices, f->chroma_v_shift); + fs->slice_width = ff_slice_coord(f, f->width , sx + sw, f->num_h_slices, f->chroma_h_shift) - fs->slice_x; + fs->slice_height = ff_slice_coord(f, f->height, sy + sh, f->num_v_slices, f->chroma_v_shift) - fs->slice_y; av_assert0((unsigned)fs->slice_width <= f->width && (unsigned)fs->slice_height <= f->height); diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 11be7fe32ea..e04695efa93 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -397,7 +397,8 @@ static int write_extradata(FFV1Context *f) int i, j, k; uint8_t state2[32][CONTEXT_SIZE]; unsigned v; - + int need_new_slices = ff_need_new_slices(f->avctx->width , f->num_h_slices, f->chroma_h_shift) + ||ff_need_new_slices(f->avctx->height, f->num_v_slices, f->chroma_v_shift); memset(state2, 128, sizeof(state2)); memset(state, 128, sizeof(state)); @@ -412,9 +413,9 @@ static int write_extradata(FFV1Context *f) put_symbol(c, state, f->version, 0); if (f->version > 2) { if (f->version == 3) { - f->micro_version = 4; + f->micro_version = 4 + need_new_slices; } else if (f->version == 4) - f->micro_version = 2; + f->micro_version = 2 + need_new_slices; put_symbol(c, state, f->micro_version, 0); } -- 2.17.1
On 07/10/2023 02:14, Michael Niedermayer wrote:
This fixes green lines in some odd dimensions with some slice configurations like Ticket 5548
This also changes the encoder and whats encoded, and would require an update to the specification. This change attempts to limit the change to configurations that have missing lines currently.
It changes a lot the count of pixels per slice, and , e.g. with 4:2:2 and 4 slices per direction (16 slices in total), 13 pixel width: before: 3/3/3/4 for luma, 2/2/2/2 for chroma (so 1 chroma too much) after: 4/4/2/3 for luma, 2/2/1/2 for chroma Wouldn't it easier for spec and maths to keep the previous behavior for luma and consider extra chroma as to be not encoded? Something like 3/3/3/4 for luma, 2/2/2/1 for chroma Actually maybe not really a change for spec in that case, more making a part more explicit while considering the patch as a bug fix rather than a spec change. Or did I miss another issue? I'll check more a bit later.
On Sat, Oct 07, 2023 at 06:52:07AM +0200, Jerome Martinez wrote:
On 07/10/2023 02:14, Michael Niedermayer wrote:
This fixes green lines in some odd dimensions with some slice configurations like Ticket 5548
This also changes the encoder and whats encoded, and would require an update to the specification. This change attempts to limit the change to configurations that have missing lines currently.
It changes a lot the count of pixels per slice, and , e.g. with 4:2:2 and 4 slices per direction (16 slices in total), 13 pixel width: before: 3/3/3/4 for luma, 2/2/2/2 for chroma (so 1 chroma too much) after: 4/4/2/3 for luma, 2/2/1/2 for chroma
Wouldn't it easier for spec and maths to keep the previous behavior for luma and consider extra chroma as to be not encoded? Something like 3/3/3/4 for luma, 2/2/2/1 for chroma Actually maybe not really a change for spec in that case, more making a part more explicit while considering the patch as a bug fix rather than a spec change.
Or did I miss another issue? I'll check more a bit later.
The existing thing has a few odd behaviors in corner cases 1. sometimes its not encoding the rightmost & bottommost chroma (leaving a green line) 2. sometimes its encoding chroma columns/rows in more than one slice (which is a waste of bits) iam not sure if the chrome that gets encoded in each slice is entirely expected, i guess it depends on what one expects The new code tries to keep slices size a multiple of the chroma subsampling factor. That way the left top coordinate of every 4:2:0 slice would always be even for example and only the last ones could have right / bottom odd when the whole image has odd width/height But this is a RFC for exactly the reasons that you mention, it changes things and would become the default in future versions Any future compression between luma and chroma planes may benefit from slices being less odd in their chroma vs luma coordinates. But noone is working on this AFAIK. Its just hypothetical thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Some people wanted to paint the bikeshed green, some blue and some pink. People argued and fought, when they finally agreed, only rust was left.
On Sat, Oct 07, 2023 at 02:14:42AM +0200, Michael Niedermayer wrote:
We always write minimal slices, the size calculation is wrong in some corner cases but as its always 1x1 (minus1) we can for now just hard-code it
This helps with ticket 5548
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ffv1enc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
will apply patch 1 and 2 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
participants (2)
-
Jerome Martinez -
Michael Niedermayer