| Message ID | 20240722094322.28916-2-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavc/ffv1dec: drop code handling AV_PIX_FMT_FLAG_PAL | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure_x86 | warning | Failed to apply patch |
On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote: > There is no reason to delay it and this is a more natural place for > this code. There is a reason. By doing it later the surrounding pixels are available and one could compute motion vectors from these surroundings and use all kinds of stuff from motion compensation and optical flow and all that. someone, i dont remember exactly who, (maybe you remember?) said something about premature optimization is the root of all evil. Here that actually applies. Moving the code up thwarts write better error concealment. (and frankly we already have most of that EC code it would just need a cleanup to free it from the 16x16 limitation) thx [...]
Quoting Michael Niedermayer (2024-07-22 23:14:04) > On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote: > > There is no reason to delay it and this is a more natural place for > > this code. > > There is a reason. > By doing it later the surrounding pixels are available and one could > compute motion vectors from these surroundings and use all kinds of stuff > from motion compensation and optical flow and all that. > > someone, i dont remember exactly who, (maybe you remember?) > said something about premature optimization is the root of all evil. > Here that actually applies. Moving the code up thwarts write better > error concealment. (and frankly we already have most of that EC code > it would just need a cleanup to free it from the 16x16 limitation) This is not an optimization though. But okay, I am dropping both original patch 28 and these 3 new patches. Are you ok with the rest of the series?
On Tue, Jul 23, 2024 at 08:52:58AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2024-07-22 23:14:04) > > On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote: > > > There is no reason to delay it and this is a more natural place for > > > this code. > > > > There is a reason. > > By doing it later the surrounding pixels are available and one could > > compute motion vectors from these surroundings and use all kinds of stuff > > from motion compensation and optical flow and all that. > > > > someone, i dont remember exactly who, (maybe you remember?) > > said something about premature optimization is the root of all evil. > > Here that actually applies. Moving the code up thwarts write better > > error concealment. (and frankly we already have most of that EC code > > it would just need a cleanup to free it from the 16x16 limitation) > > This is not an optimization though. > > But okay, I am dropping both original patch 28 and these 3 new patches. > Are you ok with the rest of the series? The series looked mostly ok, is there somewhere i can take a quick look at the current set ? also i definitly want all bugfixes and speedups you did :) thx [...]
Quoting Michael Niedermayer (2024-07-23 22:14:19) > On Tue, Jul 23, 2024 at 08:52:58AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2024-07-22 23:14:04) > > > On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote: > > > > There is no reason to delay it and this is a more natural place for > > > > this code. > > > > > > There is a reason. > > > By doing it later the surrounding pixels are available and one could > > > compute motion vectors from these surroundings and use all kinds of stuff > > > from motion compensation and optical flow and all that. > > > > > > someone, i dont remember exactly who, (maybe you remember?) > > > said something about premature optimization is the root of all evil. > > > Here that actually applies. Moving the code up thwarts write better > > > error concealment. (and frankly we already have most of that EC code > > > it would just need a cleanup to free it from the 16x16 limitation) > > > > This is not an optimization though. > > > > But okay, I am dropping both original patch 28 and these 3 new patches. > > Are you ok with the rest of the series? > > The series looked mostly ok, is there somewhere i can take a quick look > at the current set ? I just pushed it to branch 'receive_frame' in my tree.
diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index aa2c35880e..5821a4156a 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -290,7 +290,7 @@ static int decode_slice(AVCodecContext *c, void *arg) if ((p->flags & AV_FRAME_FLAG_KEY) || sc->slice_reset_contexts) { ff_ffv1_clear_slice_state(f, sc); } else if (sc->slice_damaged) { - return AVERROR_INVALIDDATA; + goto handle_damage; } width = sc->slice_width; @@ -347,6 +347,32 @@ static int decode_slice(AVCodecContext *c, void *arg) } } +handle_damage: + if (sc->slice_damaged && f->last_picture.f) { + const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(c->pix_fmt); + const uint8_t *src[4]; + uint8_t *dst[4]; + + ff_progress_frame_await(&f->last_picture, si); + + for (int j = 0; j < desc->nb_components; j++) { + int pixshift = desc->comp[j].depth > 8; + int sh = (j == 1 || j == 2) ? f->chroma_h_shift : 0; + int sv = (j == 1 || j == 2) ? f->chroma_v_shift : 0; + dst[j] = p->data[j] + p->linesize[j] * + (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift); + src[j] = f->last_picture.f->data[j] + f->last_picture.f->linesize[j] * + (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift); + + } + + av_image_copy(dst, p->linesize, src, + f->last_picture.f->linesize, + c->pix_fmt, + sc->slice_width, + sc->slice_height); + } + ff_progress_frame_report(&f->picture, si); return 0; @@ -964,31 +990,6 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *rframe, f->slice_count, sizeof(*f->slices)); - for (int i = f->slice_count - 1; i >= 0; i--) { - FFV1SliceContext *sc = &f->slices[i]; - if (sc->slice_damaged && f->last_picture.f) { - const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); - const uint8_t *src[4]; - uint8_t *dst[4]; - ff_progress_frame_await(&f->last_picture, INT_MAX); - for (int j = 0; j < desc->nb_components; j++) { - int pixshift = desc->comp[j].depth > 8; - int sh = (j == 1 || j == 2) ? f->chroma_h_shift : 0; - int sv = (j == 1 || j == 2) ? f->chroma_v_shift : 0; - dst[j] = p->data[j] + p->linesize[j] * - (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift); - src[j] = f->last_picture.f->data[j] + f->last_picture.f->linesize[j] * - (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift); - - } - - av_image_copy(dst, p->linesize, src, - f->last_picture.f->linesize, - avctx->pix_fmt, - sc->slice_width, - sc->slice_height); - } - } ff_progress_frame_report(&f->picture, INT_MAX); ff_progress_frame_unref(&f->last_picture);