[FFmpeg-devel] [PATCH v2 5/5] fftools/ffmpeg: support applying container level cropping

[Tomas Härdin]

ons 2023-07-26 klockan 19:11 -0300 skrev James Almer:
> On 7/26/2023 6:42 PM, Tomas Härdin wrote:
> > tis 2023-07-25 klockan 14:09 -0300 skrev James Almer:
> > > Signed-off-by: James Almer <jamrial at gmail.com>
> > > ---
> > > Now inserting a filter into the graph.
> > 
> > This looks useful for MXF
> > 
> > > +    { "apply_cropping",   HAS_ARG | OPT_BOOL | OPT_SPEC |
> > > +                          OPT_EXPERT |
> > > OPT_INPUT,                                { .off =
> > > OFFSET(apply_cropping) },
> > > +        "Apply frame cropping instead of exporting it" },
> > 
> > Hm. Can this be applied automatically for ffplay? When transcoding
> > I
> > expect the typical use case is to not crop and to carry the
> > metadata
> > over. MXF -> MOV for example. But when playing I expect one just
> > wants
> > to see the display rectangle.
> 
> You want it to be disabled by default on ffmpeg but enabled in
> ffplay?

Yeah that seems like it'd be most user friendly. Then 3rd party
developers can look at ffplay to see how to apply it in their own
players. mpv and vlc come to mind

> For the latter, something like:
> 
> > diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> > index 5212ad053e..217fc3e45a 100644
> > --- a/fftools/ffplay.c
> > +++ b/fftools/ffplay.c
> > @@ -36,6 +36,7 @@
> >  #include "libavutil/eval.h"
> >  #include "libavutil/mathematics.h"
> >  #include "libavutil/pixdesc.h"
> > +#include "libavutil/intreadwrite.h"
> >  #include "libavutil/imgutils.h"
> >  #include "libavutil/dict.h"
> >  #include "libavutil/fifo.h"
> > @@ -346,6 +347,7 @@ static const char **vfilters_list = NULL;
> >  static int nb_vfilters = 0;
> >  static char *afilters = NULL;
> >  static int autorotate = 1;
> > +static int apply_cropping = 1;
> >  static int find_stream_info = 1;
> >  static int filter_nbthreads = 0;
> > 
> > @@ -1922,6 +1924,27 @@ static int
> > configure_video_filters(AVFilterGraph *graph, VideoState *is, const
> > c
> >          }
> >      }
> > 
> > +    if (apply_cropping) {
> > +        size_t cropping_size;
> > +        uint8_t *cropping = av_stream_get_side_data(is->video_st,
> > AV_PKT_DATA_FRAME_CROPPING, &cropping_size);
> > +
> > +        if (cropping && cropping_size == sizeof(uint32_t) * 4) {
> > +            char crop_buf[64];
> > +            int top    = AV_RL32(cropping +  0);
> > +            int bottom = AV_RL32(cropping +  4);
> > +            int left   = AV_RL32(cropping +  8);
> > +            int right  = AV_RL32(cropping + 12);
> > +
> > +            if (top < 0 || bottom < 0 || left < 0 || right < 0)  {
> > +                ret = AVERROR(EINVAL);
> > +                goto fail;
> > +            }
> > +
> > +            snprintf(crop_buf, sizeof(crop_buf), "w=iw-%d-%d:h=ih-
> > %d-%d", left, right, top, bottom);
> > +            INSERT_FILT("crop", crop_buf);
> > +        }
> > +    }
> > +
> >      if ((ret = configure_filtergraph(graph, vfilters, filt_src,
> > last_filter)) < 0)
> >          goto fail;
> > 
> > @@ -3593,6 +3616,7 @@ static const OptionDef options[] = {
> >      { "scodec", HAS_ARG | OPT_STRING | OPT_EXPERT, {
> > &subtitle_codec_name }, "force subtitle decoder", "decoder_name" },
> >      { "vcodec", HAS_ARG | OPT_STRING | OPT_EXPERT, {   
> > &video_codec_name }, "force video decoder",    "decoder_name" },
> >      { "autorotate", OPT_BOOL, { &autorotate }, "automatically
> > rotate video", "" },
> > +    { "apply_cropping", OPT_BOOL, { &apply_cropping }, "apply
> > frame cropping", "" },
> >      { "find_stream_info", OPT_BOOL | OPT_INPUT | OPT_EXPERT, {
> > &find_stream_info },
> >          "read and decode the streams to fill missing information
> > with heuristics" },
> >      { "filter_threads", HAS_ARG | OPT_INT | OPT_EXPERT, {
> > &filter_nbthreads }, "number of filter threads per graph" },
> 
> Would do it, but i don't know if this affects the AVCodecContext
> option 
> of the same name too or not (to apply or not bitstream level
> cropping, 
> like the h264 one, which is obviously enabled by default).

Right, there's multiple levels of cropping

> To have it disabled on ffmpeg by default, i think the following would
> work (on top of this patch):
> 
> > diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c
> > index 1209cf2046..a37c136cf9 100644
> > --- a/fftools/ffmpeg_demux.c
> > +++ b/fftools/ffmpeg_demux.c
> > @@ -1086,10 +1086,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;
> > +    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);
> > +    if (ist->apply_cropping >= 0)
> > +        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) {
> > diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> > index 8cadb4732c..5df52ef718 100644
> > --- a/fftools/ffmpeg_filter.c
> > +++ b/fftools/ffmpeg_filter.c
> > @@ -1419,7 +1419,7 @@ static int
> > configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
> >              return ret;
> >      }
> > 
> > -    if (ist->apply_cropping && !(desc->flags &
> > AV_PIX_FMT_FLAG_HWACCEL)) {
> > +    if (ist->apply_cropping > 0 && !(desc->flags &
> > AV_PIX_FMT_FLAG_HWACCEL)) {
> >          size_t cropping_size;
> >          uint8_t *cropping = av_stream_get_side_data(ist->st,
> > AV_PKT_DATA_FRAME_CROPPING, &cropping_size);
> > 
> 
> And actually work fine with the AVCodecContext option.

What would be even neater is if cropping were applied automatically
when the container doesn't support such metadata. Possibly with a
message saying "container does not support display rectangle metadata.
applying cropping automatically. use -disable_cropping to disable". And
a corresponding -force_cropping. Or maybe that's too confusing? My
thinking here is that it should Just Work. If I convert an MXF file
with VBI data in the video essence, to AVI which does not have display
rectangle metadata, then I don't want to see the VBI crap in the output

/Tomas