diff mbox series

[FFmpeg-devel,1/2,v2] fftools/ffmpeg: support applying container level cropping

Message ID 20240530232251.3538-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2,v2] fftools/ffmpeg: support applying container level cropping | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 fail Make failed
andriy/make_x86 fail Make failed

Commit Message

James Almer May 30, 2024, 11:22 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 fftools/ffmpeg.h        |  7 +++++++
 fftools/ffmpeg_demux.c  | 16 ++++++++++++++++
 fftools/ffmpeg_filter.c | 10 ++++++++++
 fftools/ffmpeg_opt.c    |  3 +++
 4 files changed, 36 insertions(+)

Comments

Anton Khirnov June 25, 2024, 10:25 a.m. UTC | #1
Quoting James Almer (2024-05-31 01:22:51)
> @@ -1000,11 +1001,21 @@ int ist_filter_add(InputStream *ist, InputFilter *ifilter, int is_simple,
>      ist->filters[ist->nb_filters - 1] = ifilter;
>  
>      if (ist->par->codec_type == AVMEDIA_TYPE_VIDEO) {
> +        const AVPacketSideData *sd = av_packet_side_data_get(ist->par->coded_side_data,
> +                                                             ist->par->nb_coded_side_data,
> +                                                             AV_PKT_DATA_FRAME_CROPPING);
>          if (ist->framerate.num > 0 && ist->framerate.den > 0) {
>              opts->framerate = ist->framerate;
>              opts->flags |= IFILTER_FLAG_CFR;
>          } else
>              opts->framerate = av_guess_frame_rate(d->f.ctx, ist->st, NULL);
> +        if (sd && sd->size == sizeof(uint32_t) * 4) {
> +            opts->crop_top    = AV_RL32(sd->data +  0);
> +            opts->crop_bottom = AV_RL32(sd->data +  4);
> +            opts->crop_left   = AV_RL32(sd->data +  8);
> +            opts->crop_right  = AV_RL32(sd->data + 12);
> +            opts->flags      |= IFILTER_FLAG_CROP;
> +        }
>      } else if (ist->par->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>          /* Compute the size of the canvas for the subtitles stream.
>             If the subtitles codecpar has set a size, use it. Otherwise use the
> @@ -1241,6 +1252,9 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>      ds->autorotate = 1;
>      MATCH_PER_STREAM_OPT(autorotate, i, ds->autorotate, ic, st);
>  
> +    ds->apply_cropping = 1;
> +    MATCH_PER_STREAM_OPT(apply_cropping, i, ds->apply_cropping, ic, st);
> +
>      MATCH_PER_STREAM_OPT(codec_tags, str, codec_tag, ic, st);
>      if (codec_tag) {
>          uint32_t tag = strtol(codec_tag, &next, 0);
> @@ -1362,6 +1376,8 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>  
>      ds->dec_opts.flags |= DECODER_FLAG_BITEXACT * !!o->bitexact;
>  
> +    av_dict_set_int(&ds->decoder_opts, "apply_cropping", ds->apply_cropping, 0);

If I'm reading it right, this new option now applies only to decoder
cropping (breaking syntax, because AVOptions always take an argument),
while container cropping is always applied unconditionally.

That seems wrong.
James Almer June 25, 2024, 12:38 p.m. UTC | #2
On 6/25/2024 7:25 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-05-31 01:22:51)
>> @@ -1000,11 +1001,21 @@ int ist_filter_add(InputStream *ist, InputFilter *ifilter, int is_simple,
>>       ist->filters[ist->nb_filters - 1] = ifilter;
>>   
>>       if (ist->par->codec_type == AVMEDIA_TYPE_VIDEO) {
>> +        const AVPacketSideData *sd = av_packet_side_data_get(ist->par->coded_side_data,
>> +                                                             ist->par->nb_coded_side_data,
>> +                                                             AV_PKT_DATA_FRAME_CROPPING);
>>           if (ist->framerate.num > 0 && ist->framerate.den > 0) {
>>               opts->framerate = ist->framerate;
>>               opts->flags |= IFILTER_FLAG_CFR;
>>           } else
>>               opts->framerate = av_guess_frame_rate(d->f.ctx, ist->st, NULL);
>> +        if (sd && sd->size == sizeof(uint32_t) * 4) {
>> +            opts->crop_top    = AV_RL32(sd->data +  0);
>> +            opts->crop_bottom = AV_RL32(sd->data +  4);
>> +            opts->crop_left   = AV_RL32(sd->data +  8);
>> +            opts->crop_right  = AV_RL32(sd->data + 12);
>> +            opts->flags      |= IFILTER_FLAG_CROP;
>> +        }
>>       } else if (ist->par->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>>           /* Compute the size of the canvas for the subtitles stream.
>>              If the subtitles codecpar has set a size, use it. Otherwise use the
>> @@ -1241,6 +1252,9 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>>       ds->autorotate = 1;
>>       MATCH_PER_STREAM_OPT(autorotate, i, ds->autorotate, ic, st);
>>   
>> +    ds->apply_cropping = 1;
>> +    MATCH_PER_STREAM_OPT(apply_cropping, i, ds->apply_cropping, ic, st);
>> +
>>       MATCH_PER_STREAM_OPT(codec_tags, str, codec_tag, ic, st);
>>       if (codec_tag) {
>>           uint32_t tag = strtol(codec_tag, &next, 0);
>> @@ -1362,6 +1376,8 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>>   
>>       ds->dec_opts.flags |= DECODER_FLAG_BITEXACT * !!o->bitexact;
>>   
>> +    av_dict_set_int(&ds->decoder_opts, "apply_cropping", ds->apply_cropping, 0);
> 
> If I'm reading it right, this new option now applies only to decoder
> cropping (breaking syntax, because AVOptions always take an argument),
> while container cropping is always applied unconditionally.
> 
> That seems wrong.

Yeah, for some reason i missed a "* !!ds->apply_cropping" next to the 
IFILTER_FLAG_CROP above. With it container cropping is applied depending 
on the value of apply_cropping.

And how can i work around the ffmpeg option shadowing the avcodec one of 
the same name? Using a different name for container cropping option 
exclusively in ffmpeg is not really nice for the user. They either care 
about cropping no matter the source, or no cropping.
Anton Khirnov June 25, 2024, 1:12 p.m. UTC | #3
Quoting James Almer (2024-06-25 14:38:58)
> On 6/25/2024 7:25 AM, Anton Khirnov wrote:
> > Quoting James Almer (2024-05-31 01:22:51)
> >> @@ -1000,11 +1001,21 @@ int ist_filter_add(InputStream *ist, InputFilter *ifilter, int is_simple,
> >>       ist->filters[ist->nb_filters - 1] = ifilter;
> >>   
> >>       if (ist->par->codec_type == AVMEDIA_TYPE_VIDEO) {
> >> +        const AVPacketSideData *sd = av_packet_side_data_get(ist->par->coded_side_data,
> >> +                                                             ist->par->nb_coded_side_data,
> >> +                                                             AV_PKT_DATA_FRAME_CROPPING);
> >>           if (ist->framerate.num > 0 && ist->framerate.den > 0) {
> >>               opts->framerate = ist->framerate;
> >>               opts->flags |= IFILTER_FLAG_CFR;
> >>           } else
> >>               opts->framerate = av_guess_frame_rate(d->f.ctx, ist->st, NULL);
> >> +        if (sd && sd->size == sizeof(uint32_t) * 4) {
> >> +            opts->crop_top    = AV_RL32(sd->data +  0);
> >> +            opts->crop_bottom = AV_RL32(sd->data +  4);
> >> +            opts->crop_left   = AV_RL32(sd->data +  8);
> >> +            opts->crop_right  = AV_RL32(sd->data + 12);
> >> +            opts->flags      |= IFILTER_FLAG_CROP;
> >> +        }
> >>       } else if (ist->par->codec_type == AVMEDIA_TYPE_SUBTITLE) {
> >>           /* Compute the size of the canvas for the subtitles stream.
> >>              If the subtitles codecpar has set a size, use it. Otherwise use the
> >> @@ -1241,6 +1252,9 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
> >>       ds->autorotate = 1;
> >>       MATCH_PER_STREAM_OPT(autorotate, i, ds->autorotate, ic, st);
> >>   
> >> +    ds->apply_cropping = 1;
> >> +    MATCH_PER_STREAM_OPT(apply_cropping, i, ds->apply_cropping, ic, st);
> >> +
> >>       MATCH_PER_STREAM_OPT(codec_tags, str, codec_tag, ic, st);
> >>       if (codec_tag) {
> >>           uint32_t tag = strtol(codec_tag, &next, 0);
> >> @@ -1362,6 +1376,8 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
> >>   
> >>       ds->dec_opts.flags |= DECODER_FLAG_BITEXACT * !!o->bitexact;
> >>   
> >> +    av_dict_set_int(&ds->decoder_opts, "apply_cropping", ds->apply_cropping, 0);
> > 
> > If I'm reading it right, this new option now applies only to decoder
> > cropping (breaking syntax, because AVOptions always take an argument),
> > while container cropping is always applied unconditionally.
> > 
> > That seems wrong.
> 
> Yeah, for some reason i missed a "* !!ds->apply_cropping" next to the 
> IFILTER_FLAG_CROP above. With it container cropping is applied depending 
> on the value of apply_cropping.
> 
> And how can i work around the ffmpeg option shadowing the avcodec one of 
> the same name? Using a different name for container cropping option 
> exclusively in ffmpeg is not really nice for the user. They either care 
> about cropping no matter the source, or no cropping.

I expect plenty of broken files from various sources where the user will
want to selectively ignore either codec or container cropping.
So we could make this an enum option, with values
none/all/codec/container, mapping to 0/1/2/3 respectively. That should
keep compatibility with current syntax.
Paul B Mahol June 25, 2024, 1:23 p.m. UTC | #4
On Tue, Jun 25, 2024 at 3:12 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting James Almer (2024-06-25 14:38:58)
> > On 6/25/2024 7:25 AM, Anton Khirnov wrote:
> > > Quoting James Almer (2024-05-31 01:22:51)
> > >> @@ -1000,11 +1001,21 @@ int ist_filter_add(InputStream *ist,
> InputFilter *ifilter, int is_simple,
> > >>       ist->filters[ist->nb_filters - 1] = ifilter;
> > >>
> > >>       if (ist->par->codec_type == AVMEDIA_TYPE_VIDEO) {
> > >> +        const AVPacketSideData *sd =
> av_packet_side_data_get(ist->par->coded_side_data,
> > >> +
>  ist->par->nb_coded_side_data,
> > >> +
>  AV_PKT_DATA_FRAME_CROPPING);
> > >>           if (ist->framerate.num > 0 && ist->framerate.den > 0) {
> > >>               opts->framerate = ist->framerate;
> > >>               opts->flags |= IFILTER_FLAG_CFR;
> > >>           } else
> > >>               opts->framerate = av_guess_frame_rate(d->f.ctx,
> ist->st, NULL);
> > >> +        if (sd && sd->size == sizeof(uint32_t) * 4) {
> > >> +            opts->crop_top    = AV_RL32(sd->data +  0);
> > >> +            opts->crop_bottom = AV_RL32(sd->data +  4);
> > >> +            opts->crop_left   = AV_RL32(sd->data +  8);
> > >> +            opts->crop_right  = AV_RL32(sd->data + 12);
> > >> +            opts->flags      |= IFILTER_FLAG_CROP;
> > >> +        }
> > >>       } else if (ist->par->codec_type == AVMEDIA_TYPE_SUBTITLE) {
> > >>           /* Compute the size of the canvas for the subtitles stream.
> > >>              If the subtitles codecpar has set a size, use it.
> Otherwise use the
> > >> @@ -1241,6 +1252,9 @@ static int ist_add(const OptionsContext *o,
> Demuxer *d, AVStream *st)
> > >>       ds->autorotate = 1;
> > >>       MATCH_PER_STREAM_OPT(autorotate, i, ds->autorotate, ic, st);
> > >>
> > >> +    ds->apply_cropping = 1;
> > >> +    MATCH_PER_STREAM_OPT(apply_cropping, i, ds->apply_cropping, ic,
> st);
> > >> +
> > >>       MATCH_PER_STREAM_OPT(codec_tags, str, codec_tag, ic, st);
> > >>       if (codec_tag) {
> > >>           uint32_t tag = strtol(codec_tag, &next, 0);
> > >> @@ -1362,6 +1376,8 @@ static int ist_add(const OptionsContext *o,
> Demuxer *d, AVStream *st)
> > >>
> > >>       ds->dec_opts.flags |= DECODER_FLAG_BITEXACT * !!o->bitexact;
> > >>
> > >> +    av_dict_set_int(&ds->decoder_opts, "apply_cropping",
> ds->apply_cropping, 0);
> > >
> > > If I'm reading it right, this new option now applies only to decoder
> > > cropping (breaking syntax, because AVOptions always take an argument),
> > > while container cropping is always applied unconditionally.
> > >
> > > That seems wrong.
> >
> > Yeah, for some reason i missed a "* !!ds->apply_cropping" next to the
> > IFILTER_FLAG_CROP above. With it container cropping is applied depending
> > on the value of apply_cropping.
> >
> > And how can i work around the ffmpeg option shadowing the avcodec one of
> > the same name? Using a different name for container cropping option
> > exclusively in ffmpeg is not really nice for the user. They either care
> > about cropping no matter the source, or no cropping.
>
> I expect plenty of broken files from various sources where the user will
> want to selectively ignore either codec or container cropping.
> So we could make this an enum option, with values
> none/all/codec/container, mapping to 0/1/2/3 respectively. That should
> keep compatibility with current syntax.
>

Enum? Why not flags?


>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Anton Khirnov June 25, 2024, 1:54 p.m. UTC | #5
Quoting Paul B Mahol (2024-06-25 15:23:53)
> On Tue, Jun 25, 2024 at 3:12 PM Anton Khirnov <anton@khirnov.net> wrote:
> > I expect plenty of broken files from various sources where the user will
> > want to selectively ignore either codec or container cropping.
> > So we could make this an enum option, with values
> > none/all/codec/container, mapping to 0/1/2/3 respectively. That should
> > keep compatibility with current syntax.
> >
> 
> Enum? Why not flags?

For compatibility reasons we want 1 to be 'all'.

The other option is to special-case numeric values.
diff mbox series

Patch

diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
index fe75706afd..f908e16549 100644
--- a/fftools/ffmpeg.h
+++ b/fftools/ffmpeg.h
@@ -155,6 +155,7 @@  typedef struct OptionsContext {
     SpecifierOptList hwaccel_devices;
     SpecifierOptList hwaccel_output_formats;
     SpecifierOptList autorotate;
+    SpecifierOptList apply_cropping;
 
     /* output options */
     StreamMap *stream_maps;
@@ -239,6 +240,7 @@  enum IFilterFlags {
     IFILTER_FLAG_AUTOROTATE     = (1 << 0),
     IFILTER_FLAG_REINIT         = (1 << 1),
     IFILTER_FLAG_CFR            = (1 << 2),
+    IFILTER_FLAG_CROP           = (1 << 3),
 };
 
 typedef struct InputFilterOptions {
@@ -254,6 +256,11 @@  typedef struct InputFilterOptions {
      * accurate */
     AVRational          framerate;
 
+    unsigned crop_top;
+    unsigned crop_bottom;
+    unsigned crop_left;
+    unsigned crop_right;
+
     int                 sub2video_width;
     int                 sub2video_height;
 
diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c
index 1ca8d804ae..011b56709f 100644
--- a/fftools/ffmpeg_demux.c
+++ b/fftools/ffmpeg_demux.c
@@ -66,6 +66,7 @@  typedef struct DemuxStream {
     int                      have_sub2video;
     int                      reinit_filters;
     int                      autorotate;
+    int                      apply_cropping;
 
 
     int                      wrap_correction_done;
@@ -1000,11 +1001,21 @@  int ist_filter_add(InputStream *ist, InputFilter *ifilter, int is_simple,
     ist->filters[ist->nb_filters - 1] = ifilter;
 
     if (ist->par->codec_type == AVMEDIA_TYPE_VIDEO) {
+        const AVPacketSideData *sd = av_packet_side_data_get(ist->par->coded_side_data,
+                                                             ist->par->nb_coded_side_data,
+                                                             AV_PKT_DATA_FRAME_CROPPING);
         if (ist->framerate.num > 0 && ist->framerate.den > 0) {
             opts->framerate = ist->framerate;
             opts->flags |= IFILTER_FLAG_CFR;
         } else
             opts->framerate = av_guess_frame_rate(d->f.ctx, ist->st, NULL);
+        if (sd && sd->size == sizeof(uint32_t) * 4) {
+            opts->crop_top    = AV_RL32(sd->data +  0);
+            opts->crop_bottom = AV_RL32(sd->data +  4);
+            opts->crop_left   = AV_RL32(sd->data +  8);
+            opts->crop_right  = AV_RL32(sd->data + 12);
+            opts->flags      |= IFILTER_FLAG_CROP;
+        }
     } else if (ist->par->codec_type == AVMEDIA_TYPE_SUBTITLE) {
         /* Compute the size of the canvas for the subtitles stream.
            If the subtitles codecpar has set a size, use it. Otherwise use the
@@ -1241,6 +1252,9 @@  static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
     ds->autorotate = 1;
     MATCH_PER_STREAM_OPT(autorotate, i, ds->autorotate, ic, st);
 
+    ds->apply_cropping = 1;
+    MATCH_PER_STREAM_OPT(apply_cropping, i, ds->apply_cropping, ic, st);
+
     MATCH_PER_STREAM_OPT(codec_tags, str, codec_tag, ic, st);
     if (codec_tag) {
         uint32_t tag = strtol(codec_tag, &next, 0);
@@ -1362,6 +1376,8 @@  static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
 
     ds->dec_opts.flags |= DECODER_FLAG_BITEXACT * !!o->bitexact;
 
+    av_dict_set_int(&ds->decoder_opts, "apply_cropping", ds->apply_cropping, 0);
+
     /* Attached pics are sparse, therefore we would not want to delay their decoding
      * till EOF. */
     if (ist->st->disposition & AV_DISPOSITION_ATTACHED_PIC)
diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index 12cca684b4..f3087afc88 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -1701,6 +1701,16 @@  static int configure_input_video_filter(FilterGraph *fg, AVFilterGraph *graph,
     desc = av_pix_fmt_desc_get(ifp->format);
     av_assert0(desc);
 
+    if ((ifp->opts.flags & IFILTER_FLAG_CROP)) {
+        char crop_buf[64];
+        snprintf(crop_buf, sizeof(crop_buf), "w=iw-%d-%d:h=ih-%d-%d",
+                 ifp->opts.crop_left, ifp->opts.crop_right,
+                 ifp->opts.crop_top, ifp->opts.crop_bottom);
+        ret = insert_filter(&last_filter, &pad_idx, "crop", crop_buf);
+        if (ret < 0)
+            return ret;
+    }
+
     // TODO: insert hwaccel enabled filters like transpose_vaapi into the graph
     ifp->displaymatrix_applied = 0;
     if ((ifp->opts.flags & IFILTER_FLAG_AUTOROTATE) &&
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index 910e4a336b..0c50ce16ba 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1732,6 +1732,9 @@  const OptionDef options[] = {
     { "autoscale",                  OPT_TYPE_BOOL,   OPT_PERSTREAM | OPT_EXPERT | OPT_OUTPUT,
         { .off = OFFSET(autoscale) },
         "automatically insert a scale filter at the end of the filter graph" },
+    { "apply_cropping",             OPT_TYPE_BOOL,   OPT_PERSTREAM | OPT_EXPERT | OPT_INPUT,
+        { .off = OFFSET(apply_cropping) },
+        "apply frame cropping" },
     { "fix_sub_duration_heartbeat", OPT_TYPE_BOOL,   OPT_VIDEO | OPT_EXPERT | OPT_PERSTREAM | OPT_OUTPUT,
         { .off = OFFSET(fix_sub_duration_heartbeat) },
         "set this video output stream to be a heartbeat stream for "