Message ID | 20221001061341.662-1-ovchinnikov.dmitrii@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,crop,support,for,matroska,demuxer,1/3] libavcodec: Add crop related fields to structure AVCodecContext and AVCodecParameters. | expand |
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
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; /**