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

[Anton Khirnov <anton@khirnov.net>]

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.

-- 
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".