| Message ID | 20230719222043.59743-5-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/packet: add a decoded frame cropping side data type | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| yinshiyou/make_loongarch64 | warning | New warnings during build |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_x86 | warning | New warnings during build |
Quoting James Almer (2023-07-20 00:20:43) > diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c > index 8b750de4e5..3cf29c8b2c 100644 > --- a/fftools/ffmpeg_enc.c > +++ b/fftools/ffmpeg_enc.c > @@ -441,14 +441,16 @@ int enc_open(OutputStream *ost, AVFrame *frame) > int i; > for (i = 0; i < ist->st->nb_side_data; i++) { > AVPacketSideData *sd = &ist->st->side_data[i]; > - if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) { > + if (sd->type == AV_PKT_DATA_CPB_PROPERTIES) > + continue; > + if (ist->apply_cropping && sd->type == AV_PKT_DATA_FRAME_CROPPING) > + continue; I'm very much not a fan of the encoder doing anything based on decoder options. I know the code below already does the same thing, but I'd like to get rid of it rather than add to it.
On 7/20/2023 4:08 PM, Anton Khirnov wrote: > Quoting James Almer (2023-07-20 00:20:43) >> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c >> index 8b750de4e5..3cf29c8b2c 100644 >> --- a/fftools/ffmpeg_enc.c >> +++ b/fftools/ffmpeg_enc.c >> @@ -441,14 +441,16 @@ int enc_open(OutputStream *ost, AVFrame *frame) >> int i; >> for (i = 0; i < ist->st->nb_side_data; i++) { >> AVPacketSideData *sd = &ist->st->side_data[i]; >> - if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) { >> + if (sd->type == AV_PKT_DATA_CPB_PROPERTIES) >> + continue; >> + if (ist->apply_cropping && sd->type == AV_PKT_DATA_FRAME_CROPPING) >> + continue; > > I'm very much not a fan of the encoder doing anything based on decoder > options. Right now, all input stream side data (save for CPB) is copied to the output stream. Without this chunk, the frame cropping side data will be copied regardless of it having been applied or not at the decoding level. I don't know how else to prevent that. Maybe removing the side data from the input stream? Although that's pretty ugly. I have a separate patchset adding packet side data to codecpar and avctx, and deprecating AVStream.side_data in favor of it. With that, i could maybe use and therefore remove the cropping side data from ist->par (which is internal to the CLI) if applied. > > I know the code below already does the same thing, but I'd like to get > rid of it rather than add to it. >
Quoting James Almer (2023-07-20 21:25:02) > On 7/20/2023 4:08 PM, Anton Khirnov wrote: > > Quoting James Almer (2023-07-20 00:20:43) > >> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c > >> index 8b750de4e5..3cf29c8b2c 100644 > >> --- a/fftools/ffmpeg_enc.c > >> +++ b/fftools/ffmpeg_enc.c > >> @@ -441,14 +441,16 @@ int enc_open(OutputStream *ost, AVFrame *frame) > >> int i; > >> for (i = 0; i < ist->st->nb_side_data; i++) { > >> AVPacketSideData *sd = &ist->st->side_data[i]; > >> - if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) { > >> + if (sd->type == AV_PKT_DATA_CPB_PROPERTIES) > >> + continue; > >> + if (ist->apply_cropping && sd->type == AV_PKT_DATA_FRAME_CROPPING) > >> + continue; > > > > I'm very much not a fan of the encoder doing anything based on decoder > > options. > > Right now, all input stream side data (save for CPB) is copied to the > output stream. I think it's wrong for transcoding. Side data should be propagated through the decoder and the filtergraph and then be processed by the encoder. Just blindly copying whatever is in the input is bound to produce inaccurate information.
On 7/20/2023 4:31 PM, Anton Khirnov wrote: > Quoting James Almer (2023-07-20 21:25:02) >> On 7/20/2023 4:08 PM, Anton Khirnov wrote: >>> Quoting James Almer (2023-07-20 00:20:43) >>>> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c >>>> index 8b750de4e5..3cf29c8b2c 100644 >>>> --- a/fftools/ffmpeg_enc.c >>>> +++ b/fftools/ffmpeg_enc.c >>>> @@ -441,14 +441,16 @@ int enc_open(OutputStream *ost, AVFrame *frame) >>>> int i; >>>> for (i = 0; i < ist->st->nb_side_data; i++) { >>>> AVPacketSideData *sd = &ist->st->side_data[i]; >>>> - if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) { >>>> + if (sd->type == AV_PKT_DATA_CPB_PROPERTIES) >>>> + continue; >>>> + if (ist->apply_cropping && sd->type == AV_PKT_DATA_FRAME_CROPPING) >>>> + continue; >>> >>> I'm very much not a fan of the encoder doing anything based on decoder >>> options. >> >> Right now, all input stream side data (save for CPB) is copied to the >> output stream. > > I think it's wrong for transcoding. Side data should be propagated > through the decoder My other set will introduce this, so at least the first step towards this will be done. I'll send it in a few. > and the filtergraph and then be processed by the > encoder. It however doesn't touch lavfi. > Just blindly copying whatever is in the input is bound to > produce inaccurate information. Agree, but that's outside the scope of this set. I don't think lavfi can even take global side data right now. Only on a frame basis.
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h index f45ddf33b2..dc019b9c20 100644 --- a/fftools/ffmpeg.h +++ b/fftools/ffmpeg.h @@ -156,6 +156,8 @@ typedef struct OptionsContext { int nb_hwaccel_output_formats; SpecifierOpt *autorotate; int nb_autorotate; + SpecifierOpt *apply_cropping; + int nb_apply_cropping; /* output options */ StreamMap *stream_maps; @@ -350,6 +352,7 @@ typedef struct InputStream { int top_field_first; int autorotate; + int apply_cropping; int fix_sub_duration; diff --git a/fftools/ffmpeg_dec.c b/fftools/ffmpeg_dec.c index 5c1b8888e9..5b810f7588 100644 --- a/fftools/ffmpeg_dec.c +++ b/fftools/ffmpeg_dec.c @@ -19,6 +19,7 @@ #include "libavutil/avassert.h" #include "libavutil/dict.h" #include "libavutil/error.h" +#include "libavutil/intreadwrite.h" #include "libavutil/log.h" #include "libavutil/pixdesc.h" #include "libavutil/pixfmt.h" @@ -325,6 +326,38 @@ static int video_frame_process(InputStream *ist, AVFrame *frame) ist->dec_ctx->pix_fmt); } + if (ist->apply_cropping) { + size_t cropping_size; + uint8_t *cropping = av_stream_get_side_data(ist->st, AV_PKT_DATA_FRAME_CROPPING, &cropping_size); + if (cropping && cropping_size == sizeof(uint32_t) * 4) { + uint32_t top = AV_RL32(cropping + 0); + uint32_t bottom = AV_RL32(cropping + 4); + uint32_t left = AV_RL32(cropping + 8); + uint32_t right = AV_RL32(cropping + 12); + int err; + + if (top < INT_MAX - frame->crop_top && + bottom < INT_MAX - frame->crop_bottom && + left < INT_MAX - frame->crop_left && + right < INT_MAX - frame->crop_right) { + + frame->crop_top += top; + frame->crop_bottom += bottom; + frame->crop_left += left; + frame->crop_right += right; + + err = av_frame_apply_cropping(frame, ist->dec_ctx->flags & AV_CODEC_FLAG_UNALIGNED ? + AV_FRAME_CROP_UNALIGNED : 0); + if (err < 0) + return err; + } else + av_log(ist->dec_ctx, AV_LOG_WARNING, + "Invalid cropping information set through side data: %d/%d/%d/%d " + "(frame size %dx%d). Ignoring\n", + left, right, top, bottom, frame->width, frame->height); + } + } + if(ist->top_field_first>=0) frame->flags |= AV_FRAME_FLAG_TOP_FIELD_FIRST; diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c index 9e7f13a2b4..09d040b626 100644 --- a/fftools/ffmpeg_demux.c +++ b/fftools/ffmpeg_demux.c @@ -48,6 +48,7 @@ static const char *const opt_name_hwaccels[] = {"hwaccel", NULL static const char *const opt_name_hwaccel_devices[] = {"hwaccel_device", NULL}; static const char *const opt_name_hwaccel_output_formats[] = {"hwaccel_output_format", NULL}; static const char *const opt_name_autorotate[] = {"autorotate", NULL}; +static const char *const opt_name_apply_cropping[] = {"apply_cropping", NULL}; static const char *const opt_name_display_rotations[] = {"display_rotation", NULL}; static const char *const opt_name_display_hflips[] = {"display_hflip", NULL}; static const char *const opt_name_display_vflips[] = {"display_vflip", NULL}; @@ -1068,6 +1069,11 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st) ist->autorotate = 1; MATCH_PER_STREAM_OPT(autorotate, i, ist->autorotate, ic, st); + ist->apply_cropping = 1; + MATCH_PER_STREAM_OPT(apply_cropping, i, ist->apply_cropping, ic, st); + + av_dict_set_int(&o->g->codec_opts, "apply_cropping", ist->apply_cropping, 0); + MATCH_PER_STREAM_OPT(codec_tags, str, codec_tag, ic, st); if (codec_tag) { uint32_t tag = strtol(codec_tag, &next, 0); diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c index 8b750de4e5..3cf29c8b2c 100644 --- a/fftools/ffmpeg_enc.c +++ b/fftools/ffmpeg_enc.c @@ -441,14 +441,16 @@ int enc_open(OutputStream *ost, AVFrame *frame) int i; for (i = 0; i < ist->st->nb_side_data; i++) { AVPacketSideData *sd = &ist->st->side_data[i]; - if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) { + if (sd->type == AV_PKT_DATA_CPB_PROPERTIES) + continue; + if (ist->apply_cropping && sd->type == AV_PKT_DATA_FRAME_CROPPING) + continue; uint8_t *dst = av_stream_new_side_data(ost->st, sd->type, sd->size); if (!dst) return AVERROR(ENOMEM); memcpy(dst, sd->data, sd->size); if (ist->autorotate && sd->type == AV_PKT_DATA_DISPLAYMATRIX) av_display_rotation_set((int32_t *)dst, 0); - } } } diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 300f042660..fe61a73643 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -1677,6 +1677,9 @@ const OptionDef options[] = { { "autorotate", HAS_ARG | OPT_BOOL | OPT_SPEC | OPT_EXPERT | OPT_INPUT, { .off = OFFSET(autorotate) }, "automatically insert correct rotate filters" }, + { "apply_cropping", HAS_ARG | OPT_BOOL | OPT_SPEC | + OPT_EXPERT | OPT_INPUT, { .off = OFFSET(apply_cropping) }, + "Apply frame cropping instead of exporting it" }, { "autoscale", HAS_ARG | OPT_BOOL | OPT_SPEC | OPT_EXPERT | OPT_OUTPUT, { .off = OFFSET(autoscale) }, "automatically insert a scale filter at the end of the filter graph" },
Signed-off-by: James Almer <jamrial@gmail.com> --- fftools/ffmpeg.h | 3 +++ fftools/ffmpeg_dec.c | 33 +++++++++++++++++++++++++++++++++ fftools/ffmpeg_demux.c | 6 ++++++ fftools/ffmpeg_enc.c | 6 ++++-- fftools/ffmpeg_opt.c | 3 +++ 5 files changed, 49 insertions(+), 2 deletions(-)