| Message ID | 20221007145942.880-1-ovchinnikov.dmitrii@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,crop,support,for,matroska,demuxer,V3,1/3] libavcodec: Add crop related fields to structure AVCodecContext and AVCodecParameters. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting OvchinnikovDmitrii (2022-10-07 16:59:40) > --- > libavcodec/avcodec.h | 35 +++++++++++++++++++++++++++++++++++ > libavcodec/codec_par.h | 8 ++++++++ > libavcodec/options_table.h | 4 ++++ > 3 files changed, 47 insertions(+) > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 7365eb5cc0..d28a7cc022 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -380,6 +380,19 @@ typedef struct RcOverride{ > */ > #define AV_GET_ENCODE_BUFFER_FLAG_REF (1 << 0) > > +/** > +* Video decoding only. Certain container support cropping, meaning that > +* only a sub-rectangle of the decoded frame is intended for display. > +* Certain codec supports cropping as well.This option controls how > +* cropping is handled by libavcodec when container cropping and > +* codec cropping exist. > +*/ > +enum CONTAINER_CROPPING_POLICY_TYPE { > + FF_CONTAINER_CROPPING_IGNORE = 0, > + FF_CONTAINER_CROPPING_ADDITION, > + FF_CONTAINER_CROPPING_OVERWRITE > +}; > + > struct AVCodecInternal; > > /** > @@ -2057,6 +2070,28 @@ typedef struct AVCodecContext { > * The decoder can then override during decoding as needed. > */ > AVChannelLayout ch_layout; > + > + /* 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 > + */ > + enum CONTAINER_CROPPING_POLICY_TYPE container_apply_cropping; > + > + /** > + * The cropping parameters from container. > + */ > + int container_crop_top; > + int container_crop_left; > + int container_crop_bottom; > + int container_crop_right; I don't see why should a decoder deal with this at all? This can be just as well done by the caller.
On 07/10/2022 16:59, OvchinnikovDmitrii wrote: > --- > libavcodec/avcodec.h | 35 +++++++++++++++++++++++++++++++++++ > libavcodec/codec_par.h | 8 ++++++++ > libavcodec/options_table.h | 4 ++++ > 3 files changed, 47 insertions(+) > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 7365eb5cc0..d28a7cc022 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -380,6 +380,19 @@ typedef struct RcOverride{ > */ > #define AV_GET_ENCODE_BUFFER_FLAG_REF (1 << 0) > > +/** > +* Video decoding only. Certain container support cropping, meaning that > +* only a sub-rectangle of the decoded frame is intended for display. > +* Certain codec supports cropping as well.This option controls how > +* cropping is handled by libavcodec when container cropping and > +* codec cropping exist. > +*/ > +enum CONTAINER_CROPPING_POLICY_TYPE { > + FF_CONTAINER_CROPPING_IGNORE = 0, > + FF_CONTAINER_CROPPING_ADDITION, > + FF_CONTAINER_CROPPING_OVERWRITE > +}; > + > struct AVCodecInternal; > > /** > @@ -2057,6 +2070,28 @@ typedef struct AVCodecContext { > * The decoder can then override during decoding as needed. > */ > AVChannelLayout ch_layout; > + > + /* 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 > + */ > + enum CONTAINER_CROPPING_POLICY_TYPE container_apply_cropping; > + > + /** > + * The cropping parameters from container. > + */ > + int container_crop_top; > + int container_crop_left; > + int container_crop_bottom; > + int container_crop_right; > } AVCodecContext; > > /** > diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h > index 7660791a12..12d8ceb521 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 cropping parameters from container. > + */ > + int container_crop_top; > + int container_crop_left; > + int container_crop_bottom; > + int container_crop_right; > } AVCodecParameters; I really don't like seeing this in avcodec. Why does an encoder or decoder need to care about this, given it's container level information? Do you plan on implementing cropping support into all the encoders? This should be handled via an avfilter, which might get the cropping info from the demuxer via site data or something.
On 10/11/2022 9:43 AM, Timo Rothenpieler wrote: > > > On 07/10/2022 16:59, OvchinnikovDmitrii wrote: >> --- >> libavcodec/avcodec.h | 35 +++++++++++++++++++++++++++++++++++ >> libavcodec/codec_par.h | 8 ++++++++ >> libavcodec/options_table.h | 4 ++++ >> 3 files changed, 47 insertions(+) >> >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index 7365eb5cc0..d28a7cc022 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -380,6 +380,19 @@ typedef struct RcOverride{ >> */ >> #define AV_GET_ENCODE_BUFFER_FLAG_REF (1 << 0) >> +/** >> +* Video decoding only. Certain container support cropping, meaning that >> +* only a sub-rectangle of the decoded frame is intended for display. >> +* Certain codec supports cropping as well.This option controls how >> +* cropping is handled by libavcodec when container cropping and >> +* codec cropping exist. >> +*/ >> +enum CONTAINER_CROPPING_POLICY_TYPE { >> + FF_CONTAINER_CROPPING_IGNORE = 0, >> + FF_CONTAINER_CROPPING_ADDITION, >> + FF_CONTAINER_CROPPING_OVERWRITE >> +}; >> + >> struct AVCodecInternal; >> /** >> @@ -2057,6 +2070,28 @@ typedef struct AVCodecContext { >> * The decoder can then override during decoding as >> needed. >> */ >> AVChannelLayout ch_layout; >> + >> + /* 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 >> + */ >> + enum CONTAINER_CROPPING_POLICY_TYPE container_apply_cropping; >> + >> + /** >> + * The cropping parameters from container. >> + */ >> + int container_crop_top; >> + int container_crop_left; >> + int container_crop_bottom; >> + int container_crop_right; >> } AVCodecContext; >> /** >> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h >> index 7660791a12..12d8ceb521 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 cropping parameters from container. >> + */ >> + int container_crop_top; >> + int container_crop_left; >> + int container_crop_bottom; >> + int container_crop_right; >> } AVCodecParameters; > > I really don't like seeing this in avcodec. > Why does an encoder or decoder need to care about this, given it's > container level information? > > Do you plan on implementing cropping support into all the encoders? > This should be handled via an avfilter, which might get the cropping > info from the demuxer via site data or something. There's logic in decode.c to apply bitstream level cropping (like in h264). This could be extended to also apply container level cropping exported as packet side data, like you suggested. How are we handling stream side data, for that matter? Apparently injected only on the first output packet, which is then converted into frame side data in lavc, and only done by ffplay.c?
>> I don't see why should a decoder deal with this at all? >>This can be just as well done by the caller. This implementation takes a reference of the h.264 supporting cropping. In h264, if width (stored in ACCodecContext) parsed from container is different from width parsed from elementary, it will trigger cropping. This is essentially a kind of cropping that related with container. It will infer parameters cb(crop bottom), cr(crop right), and then finally assigned them to AVFrame. This gives the inspiration when we get direct cropping parameters from container. We can us the same methodology. Parser cropping parameters from container in demuxer -> stored container cropping parameters in AVCodec -> pass container cropping parameters to AVFrame-> Apply Crop in frame.c of libavutil. I agree that the caller decide container cropping. To support caller decide the behaviors of cropping,I declare a new field “container_apply_cropping”. The value of this fields container_apply_cropping is decided by caller. This field is by default 1. - 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) Caller can total decide the behaviors of container cropping. This field “container_apply_cropping” subject the value of “apply_cropping" (an existing field ). Only apply_cropping is 1, this field works. This make sure the container cropping parameters supporting is consistent with the current cropping mechanism.
>>Why does an encoder or decoder need to care about this, given it's >>container level information? >>Do you plan on implementing cropping support into all the encoders? This implementation takes a reference of the h.264 supporting cropping. In h264, if width (stored in ACCodecContext) parsed from container is different from width parsed from elementary, it will trigger cropping. This is essentially a kind of cropping that related with container. It will infer parameters cb(crop bottom), cr(crop right), and then finally assigned them to AVFrame. This gives the inspiration when we get direct cropping parameters from container. We can us the same methodology. Parser cropping parameters from container in demuxer -> stored container cropping parameters in AVCodec -> pass container cropping parameters to AVFrame-> Apply Crop in frame.c of libavutil. Container cropping can both be implemented in libavutil, or libavfilter. The as-is implementation of H.264 gave a detailed implement reference that we can put cropping in libavcode and libavutil. My implementation just follows that methodology. From the another point of view, there are some benefit as well: cropping is a light weight process (only copy yuv data), comparing with some module such as edge_common, colorspacedsp etc. in libavfilter. Put container cropping in libavcodec and libavutil is very concise. Currently we are supporting the matroska codec. Matroska will be the first one that use submitted code in decoder.c and AVCodecContext to support container cropping. Because container cropping parameters I added are 0 by default, it will not affect any existing container or future container which don’t support crop. If other developers plan to add container crop supporting for certain container, those codes in public section (such as codes in in decoder.c and AVCodecContext ) are possible shared by them.
Quoting James Almer (2022-10-11 14:51:34) > On 10/11/2022 9:43 AM, Timo Rothenpieler wrote: > > I really don't like seeing this in avcodec. > > Why does an encoder or decoder need to care about this, given it's > > container level information? > > > > Do you plan on implementing cropping support into all the encoders? > > This should be handled via an avfilter, which might get the cropping > > info from the demuxer via site data or something. > > There's logic in decode.c to apply bitstream level cropping (like in > h264). This could be extended to also apply container level cropping > exported as packet side data, like you suggested. That logic is essentially just calling av_frame_apply_cropping(). Callers can just as easily do this themselves; adding a bunch of new fields and a system of policies seems like unnecessary complexity that cannot provide all the flexibility one might want anyway.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 7365eb5cc0..d28a7cc022 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -380,6 +380,19 @@ typedef struct RcOverride{ */ #define AV_GET_ENCODE_BUFFER_FLAG_REF (1 << 0) +/** +* Video decoding only. Certain container support cropping, meaning that +* only a sub-rectangle of the decoded frame is intended for display. +* Certain codec supports cropping as well.This option controls how +* cropping is handled by libavcodec when container cropping and +* codec cropping exist. +*/ +enum CONTAINER_CROPPING_POLICY_TYPE { + FF_CONTAINER_CROPPING_IGNORE = 0, + FF_CONTAINER_CROPPING_ADDITION, + FF_CONTAINER_CROPPING_OVERWRITE +}; + struct AVCodecInternal; /** @@ -2057,6 +2070,28 @@ typedef struct AVCodecContext { * The decoder can then override during decoding as needed. */ AVChannelLayout ch_layout; + + /* 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 + */ + enum CONTAINER_CROPPING_POLICY_TYPE container_apply_cropping; + + /** + * The cropping parameters from container. + */ + int container_crop_top; + int container_crop_left; + int container_crop_bottom; + int container_crop_right; } AVCodecContext; /** diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h index 7660791a12..12d8ceb521 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 cropping parameters from container. + */ + int container_crop_top; + int container_crop_left; + int container_crop_bottom; + int container_crop_right; } AVCodecParameters; /** diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h index cd02f5096f..fd1ef21f90 100644 --- a/libavcodec/options_table.h +++ b/libavcodec/options_table.h @@ -401,6 +401,10 @@ static const AVOption avcodec_options[] = { {"allow_profile_mismatch", "attempt to decode anyway if HW accelerated decoder's supported profiles do not exactly match the stream", 0, AV_OPT_TYPE_CONST, {.i64 = AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH }, INT_MIN, INT_MAX, V | D, "hwaccel_flags"}, {"extra_hw_frames", "Number of extra hardware frames to allocate for the user", OFFSET(extra_hw_frames), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, V|D }, {"discard_damaged_percentage", "Percentage of damaged samples to discard a frame", OFFSET(discard_damaged_percentage), AV_OPT_TYPE_INT, {.i64 = 95 }, 0, 100, V|D }, +{ "container_apply_cropping", "ploicy using container cropping parameters", OFFSET(container_apply_cropping), AV_OPT_TYPE_INT64, {.i64 = FF_CONTAINER_CROPPING_ADDITION }, 0, 2, V | D, "container_apply_cropping" }, +{ "ignore", "ignore container cropping", 0, AV_OPT_TYPE_CONST, {.i64 = FF_CONTAINER_CROPPING_IGNORE }, 0, 0, V | D, "container_apply_cropping" }, +{ "addition", "apply container cropping additionally to elementary stream cropping", 0, AV_OPT_TYPE_CONST, {.i64 = FF_CONTAINER_CROPPING_ADDITION }, 0, 0, V | D, "container_apply_cropping" }, +{ "overwrite", "use container cropping to overwrite elementary stream cropping", 0, AV_OPT_TYPE_CONST, {.i64 = FF_CONTAINER_CROPPING_OVERWRITE }, 0, 0, V | D, "container_apply_cropping" }, {NULL}, };