[FFmpeg-devel,crop,support,for,matroska,demuxer,1/3] libavcodec: Add crop related fields to structure AVCodecContext and AVCodecParameters.

---
 libavcodec/avcodec.h   | 8 ++++++++
 libavcodec/codec_par.h | 8 ++++++++
 2 files changed, 16 insertions(+)

On Sat, Oct 1, 2022 at 8:14 AM OvchinnikovDmitrii
<ovchinnikov.dmitrii@gmail.com> wrote:
>
> ---
>  libavcodec/avcodec.h   | 8 ++++++++
>  libavcodec/codec_par.h | 8 ++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7365eb5cc0..66df571afc 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -585,6 +585,14 @@ typedef struct AVCodecContext {
>       */
>      int coded_width, coded_height;
>
> +    /**
> +     * The dimensions of the crop, usually from container.
> +     */
> +    int crop_top;
> +    int crop_left;
> +    int crop_bottom;
> +    int crop_right;
> +
>      /**
>       * the number of pictures in a group of pictures, or 0 for intra_only
>       * - encoding: Set by user.
> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> index 7660791a12..c730a79957 100644
> --- a/libavcodec/codec_par.h
> +++ b/libavcodec/codec_par.h
> @@ -210,6 +210,14 @@ typedef struct AVCodecParameters {
>       * Audio only. The channel layout and number of channels.
>       */
>      AVChannelLayout ch_layout;
> +
> +    /**
> +     * The dimensions of the crop, usually from container.
> +     */
> +    int crop_top;
> +    int crop_left;
> +    int crop_bottom;
> +    int crop_right;
>  } AVCodecParameters;
>

All of these should be size_t (and in AVCodecContext as well,
naturally), matching the type in AVFrame.

- Hendrik

On 01.10.2022 08:13, OvchinnikovDmitrii wrote:
> ---
>   libavcodec/avcodec.h   | 8 ++++++++
>   libavcodec/codec_par.h | 8 ++++++++
>   2 files changed, 16 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7365eb5cc0..66df571afc 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -585,6 +585,14 @@ typedef struct AVCodecContext {
>        */
>       int coded_width, coded_height;
>   
> +    /**
> +     * The dimensions of the crop, usually from container.
> +     */
> +    int crop_top;
> +    int crop_left;
> +    int crop_bottom;
> +    int crop_right;
> +

Shouldn't these be added at the very end, to not break ABI?

I'm also not very convinced this kind of information really belongs into 
AVCodecContext and codecpar.
Can't it just be frame-sidedata?

Quoting Timo Rothenpieler (2022-10-01 13:24:26)
> On 01.10.2022 08:13, OvchinnikovDmitrii wrote:
> > ---
> >   libavcodec/avcodec.h   | 8 ++++++++
> >   libavcodec/codec_par.h | 8 ++++++++
> >   2 files changed, 16 insertions(+)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 7365eb5cc0..66df571afc 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -585,6 +585,14 @@ typedef struct AVCodecContext {
> >        */
> >       int coded_width, coded_height;
> >   
> > +    /**
> > +     * The dimensions of the crop, usually from container.
> > +     */
> > +    int crop_top;
> > +    int crop_left;
> > +    int crop_bottom;
> > +    int crop_right;
> > +
> 
> Shouldn't these be added at the very end, to not break ABI?
> 

yes they should

> I'm also not very convinced this kind of information really belongs into 
> AVCodecContext and codecpar.
> Can't it just be frame-sidedata?

AVFrame already has fields for this, which are typically filled from
codec-level headers.

The idea here is to support container-level information, so some new
fields for lavf are certainly needed, but I'm not convinced just adding
them to AVCodecContext is the best solution. The semantics of these new
fields definitely need to be defined more precisely (like who sets them
and what does lavc do with the values).

On 10/1/2022 4:06 AM, Hendrik Leppkes wrote:
> On Sat, Oct 1, 2022 at 8:14 AM OvchinnikovDmitrii
> <ovchinnikov.dmitrii@gmail.com> wrote:
>>
>> ---
>>   libavcodec/avcodec.h   | 8 ++++++++
>>   libavcodec/codec_par.h | 8 ++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 7365eb5cc0..66df571afc 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -585,6 +585,14 @@ typedef struct AVCodecContext {
>>        */
>>       int coded_width, coded_height;
>>
>> +    /**
>> +     * The dimensions of the crop, usually from container.
>> +     */
>> +    int crop_top;
>> +    int crop_left;
>> +    int crop_bottom;
>> +    int crop_right;
>> +
>>       /**
>>        * the number of pictures in a group of pictures, or 0 for intra_only
>>        * - encoding: Set by user.
>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
>> index 7660791a12..c730a79957 100644
>> --- a/libavcodec/codec_par.h
>> +++ b/libavcodec/codec_par.h
>> @@ -210,6 +210,14 @@ typedef struct AVCodecParameters {
>>        * Audio only. The channel layout and number of channels.
>>        */
>>       AVChannelLayout ch_layout;
>> +
>> +    /**
>> +     * The dimensions of the crop, usually from container.
>> +     */
>> +    int crop_top;
>> +    int crop_left;
>> +    int crop_bottom;
>> +    int crop_right;
>>   } AVCodecParameters;
>>
> 
> All of these should be size_t (and in AVCodecContext as well,
> naturally), matching the type in AVFrame.

IMO the AVFrame ones should have not been size_t to begin with, not just 
because the actual dimensions you'll apply them to are int, but because 
these fields are not arch dependent or meant for the size of some object 
in memory.

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

Sent an updated version:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=7707.
1)
>>All of these should be size_t
The field type is still "int" according to James Almer's comment.
2) I placed the fields at the very end, according to a message from Timo
Rothenpieler.
3)
>>The semantics of these new fields definitely need to be defined more
precisely
Added a flag responsible for the behavior(enum
CONTAINER_CROPPING_POLICY_TYPE container_apply_cropping;).
When set to 1 (the default), libavcodec will apply container cropping
to codec cropping additionally.
When set to 2, libavcodec will use container cropping to overwrite
codec cropping (the final cropping uses container cropping parameters)
When set to 0, libavcodec will ignore container cropping parameters
(the final cropping uses codec cropping parameters)This field works with
"apply_cropping".
Only if apply_cropping is 1, this field works