| Message ID | 20200418101415.26409-7-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/9] libavutil: add API for exporting video frame quantizers | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: [...] > diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > index 3d0d4969b8..cb7ce7a158 100644 > --- a/tests/fate/filter-video.mak > +++ b/tests/fate/filter-video.mak > @@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 > FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) > $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd > > -fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > +fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" ffmpeg / ffplay should automatically enable the exportation of the parameters when theres a filter downstream that needs such parameters Otherwise the use of these filters (and other filters that need any kind of information thats unavailable by default) would become a bit akward to use thx [...]
On Sat, 18 Apr 2020, Michael Niedermayer wrote: > On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: > [...] >> diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak >> index 3d0d4969b8..cb7ce7a158 100644 >> --- a/tests/fate/filter-video.mak >> +++ b/tests/fate/filter-video.mak >> @@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 >> FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) >> $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd >> >> -fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" >> +fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > > ffmpeg / ffplay should automatically enable the exportation of the parameters > when theres a filter downstream that needs such parameters > > Otherwise the use of these filters (and other filters that need any kind of > information thats unavailable by default) would become a bit akward to use Why? It is not unusual at all that you need to specify certain extra parameters to export some kind of metadata. Isn't this just another example of that? Or are you worried that existing command lines won't work anymore without specifying the new flag? Thanks, Marton
Am Sa., 18. Apr. 2020 um 19:03 Uhr schrieb Marton Balint <cus@passwd.hu>: > > On Sat, 18 Apr 2020, Michael Niedermayer wrote: > > > On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: > > [...] > >> diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > >> index 3d0d4969b8..cb7ce7a158 100644 > >> --- a/tests/fate/filter-video.mak > >> +++ b/tests/fate/filter-video.mak > >> @@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 > >> FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) > >> $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd > >> > >> -fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > >> +fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > > > > ffmpeg / ffplay should automatically enable the exportation of the parameters > > when theres a filter downstream that needs such parameters > > > > Otherwise the use of these filters (and other filters that need any kind of > > information thats unavailable by default) would become a bit akward to use > > Why? It is not unusual at all that you need to specify certain extra > parameters to export some kind of metadata. Isn't this just another > example of that? But you are talking about command line that simply don't work, or do I misunderstand? > Or are you worried that existing command lines won't work > anymore without specifying the new flag? That's also true afaict. Carl Eugen
On Sat, Apr 18, 2020 at 07:03:16PM +0200, Marton Balint wrote: > > > On Sat, 18 Apr 2020, Michael Niedermayer wrote: > > >On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: > >[...] > >>diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > >>index 3d0d4969b8..cb7ce7a158 100644 > >>--- a/tests/fate/filter-video.mak > >>+++ b/tests/fate/filter-video.mak > >>@@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 > >> FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) > >> $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd > >> > >>-fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > >>+fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > > > >ffmpeg / ffplay should automatically enable the exportation of the parameters > >when theres a filter downstream that needs such parameters > > > >Otherwise the use of these filters (and other filters that need any kind of > >information thats unavailable by default) would become a bit akward to use > > Why? It is not unusual at all that you need to specify certain extra > parameters to export some kind of metadata. in addition to what carl said. It seems you consider that "normal", i wouldnt really consider it normal to have to specify this by hand. All the information is there for the code to do this automatically and its not really a A vs B alternative. Instead "-export_side_data venc_params" with a filter using this afterwards is the only functional choice And this is not just useful here there are other situations where this is useful as you said "not unusual at all that you need to specify certain extra parameters to export some kind of metadata" IMHO, let the machiene do this kind of work not the user Thanks [...]
On Sat, 18 Apr 2020, Michael Niedermayer wrote: > On Sat, Apr 18, 2020 at 07:03:16PM +0200, Marton Balint wrote: >> >> >> On Sat, 18 Apr 2020, Michael Niedermayer wrote: >> >>> On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: >>> [...] >>>> diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak >>>> index 3d0d4969b8..cb7ce7a158 100644 >>>> --- a/tests/fate/filter-video.mak >>>> +++ b/tests/fate/filter-video.mak >>>> @@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 >>>> FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) >>>> $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd >>>> >>>> -fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" >>>> +fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" >>> >>> ffmpeg / ffplay should automatically enable the exportation of the parameters >>> when theres a filter downstream that needs such parameters >>> >>> Otherwise the use of these filters (and other filters that need any kind of >>> information thats unavailable by default) would become a bit akward to use >> >> Why? It is not unusual at all that you need to specify certain extra >> parameters to export some kind of metadata. > > in addition to what carl said. > It seems you consider that "normal", i wouldnt really consider it normal > to have to specify this by hand. > All the information is there for the code to do this automatically and its > not really a A vs B alternative. Instead "-export_side_data venc_params" > with a filter using this afterwards is the only functional choice And how will you decide if a pp filter in a complex filtergraph is actually processing your video input or not? Or how will you know that the pp filter is parametrized in a way that it wants to use motion vectors? This not looks to me easy at all. Automagically enabling this would look a lot more like a heuristic to me, a case when the application wants to be smarter than it actually is. Regards, Marton
On Sat, Apr 18, 2020 at 09:07:09PM +0200, Marton Balint wrote: > > > On Sat, 18 Apr 2020, Michael Niedermayer wrote: > > >On Sat, Apr 18, 2020 at 07:03:16PM +0200, Marton Balint wrote: > >> > >> > >>On Sat, 18 Apr 2020, Michael Niedermayer wrote: > >> > >>>On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: > >>>[...] > >>>>diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > >>>>index 3d0d4969b8..cb7ce7a158 100644 > >>>>--- a/tests/fate/filter-video.mak > >>>>+++ b/tests/fate/filter-video.mak > >>>>@@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 > >>>>FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) > >>>>$(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd > >>>> > >>>>-fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > >>>>+fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > >>> > >>>ffmpeg / ffplay should automatically enable the exportation of the parameters > >>>when theres a filter downstream that needs such parameters > >>> > >>>Otherwise the use of these filters (and other filters that need any kind of > >>>information thats unavailable by default) would become a bit akward to use > >> > >>Why? It is not unusual at all that you need to specify certain extra > >>parameters to export some kind of metadata. > > > >in addition to what carl said. > >It seems you consider that "normal", i wouldnt really consider it normal > >to have to specify this by hand. > >All the information is there for the code to do this automatically and its > >not really a A vs B alternative. Instead "-export_side_data venc_params" > >with a filter using this afterwards is the only functional choice > > And how will you decide if a pp filter in a complex filtergraph is actually > processing your video input or not? Or how will you know that the pp filter > is parametrized in a way that it wants to use motion vectors? > > This not looks to me easy at all. Automagically enabling this would look a > lot more like a heuristic to me, a case when the application wants to be > smarter than it actually is. the filter requests the metadata types from its input from which it wants it. the prior filter passes the request on exactly the same but in the opposit direction to how it will pass frames metadata. In reality for just pp* its much simpler actually as pp filters will be after the decoder no scale or crop even will be between because these would cause problems. one could imagine a split before a pp but thats very easy to pass back. there would not be something like a overlay before pp as again this wouldnt work without compensating the passed metadata (which is not done ATM). You can also do it in a completely other way 1. Enable all export initially 2. Pass in AVFrames a way so that every subsequent consumer can mark each data type as used or not used. 3. if frames get deallocated with some side/metadata never used the decoder can stop exporting that in future frames Theres also the simpler solution of simply giving each filter a flag for each exportable type of data and then have every decoder feeding a graph with a set flag to export that type of data. This would export more than needed in case of complex graphs Iam sure there are more solutions and iam sure ive missed some aspects of what i suggested above. But it seems to me this is a useful feature beyond this case here Thanks [...]
Quoting Michael Niedermayer (2020-04-18 21:52:22) > On Sat, Apr 18, 2020 at 09:07:09PM +0200, Marton Balint wrote: > > > > > > On Sat, 18 Apr 2020, Michael Niedermayer wrote: > > > > >On Sat, Apr 18, 2020 at 07:03:16PM +0200, Marton Balint wrote: > > >> > > >> > > >>On Sat, 18 Apr 2020, Michael Niedermayer wrote: > > >> > > >>>On Sat, Apr 18, 2020 at 12:14:12PM +0200, Anton Khirnov wrote: > > >>>[...] > > >>>>diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > > >>>>index 3d0d4969b8..cb7ce7a158 100644 > > >>>>--- a/tests/fate/filter-video.mak > > >>>>+++ b/tests/fate/filter-video.mak > > >>>>@@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 > > >>>>FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) > > >>>>$(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd > > >>>> > > >>>>-fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > > >>>>+fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" > > >>> > > >>>ffmpeg / ffplay should automatically enable the exportation of the parameters > > >>>when theres a filter downstream that needs such parameters > > >>> > > >>>Otherwise the use of these filters (and other filters that need any kind of > > >>>information thats unavailable by default) would become a bit akward to use > > >> > > >>Why? It is not unusual at all that you need to specify certain extra > > >>parameters to export some kind of metadata. > > > > > >in addition to what carl said. > > >It seems you consider that "normal", i wouldnt really consider it normal > > >to have to specify this by hand. > > >All the information is there for the code to do this automatically and its > > >not really a A vs B alternative. Instead "-export_side_data venc_params" > > >with a filter using this afterwards is the only functional choice > > > > And how will you decide if a pp filter in a complex filtergraph is actually > > processing your video input or not? Or how will you know that the pp filter > > is parametrized in a way that it wants to use motion vectors? > > > > This not looks to me easy at all. Automagically enabling this would look a > > lot more like a heuristic to me, a case when the application wants to be > > smarter than it actually is. > > the filter requests the metadata types from its input from which it wants it. > the prior filter passes the request on exactly the same but in the opposit > direction to how it will pass frames metadata. > > In reality for just pp* its much simpler actually as pp filters will be after the decoder > no scale or crop even will be between because these would cause problems. > one could imagine a split before a pp but thats very easy to pass back. > there would not be something like a overlay before pp as again this wouldnt > work without compensating the passed metadata (which is not done ATM). > > You can also do it in a completely other way > 1. Enable all export initially > 2. Pass in AVFrames a way so that every subsequent consumer can mark each > data type as used or not used. > 3. if frames get deallocated with some side/metadata never used the decoder > can stop exporting that in future frames > > Theres also the simpler solution of simply giving each filter a flag for each > exportable type of data and then have every decoder feeding a graph with a > set flag to export that type of data. > This would export more than needed in case of complex graphs > > Iam sure there are more solutions and iam sure ive missed some aspects of > what i suggested above. But it seems to me this is a useful feature beyond > this case here I agree with Marton that this would be the case of a program trying to be smarter than it is, which is IMO not appropriate here. ffmpeg.c is suppposed to be, inasmuch as possible, a generic transcoder. That design choice means that it is very powerful, but the corresponding drawback is that it requires a bit more effort to make it do exactly what you want it to do. I believe that trying to make it work "automagically" for all imaginable use cases - will not work anyway because there are more of those than anyone can handle - will cause the code to become a convoluted impossible-to-understand mess that breaks all the time for inexplicable reasons In this case specifically, there is no guarantee that the mere presence of the pp filter implies that the user wants the decoder to export this metadata. Maybe the user wants to generate the tables manually with the qp filter. Or load them from some other source that will be added in the future. Other possibilities are also imaginable. Furthermore, pp is not the only user of the QP tables. Is ffmpeg.c to maintain a constantly-changing list of filters and magically conjure up decoder flags based on those? All my experience tells me this will only lead to pain.
diff --git a/libavfilter/vf_pp.c b/libavfilter/vf_pp.c index 524ef1bb0a..87eef4561f 100644 --- a/libavfilter/vf_pp.c +++ b/libavfilter/vf_pp.c @@ -26,6 +26,8 @@ #include "libavutil/avassert.h" #include "libavutil/opt.h" +#include "libavutil/video_enc_params.h" + #include "internal.h" #include "libpostproc/postprocess.h" @@ -118,6 +120,41 @@ static int pp_config_props(AVFilterLink *inlink) return 0; } +static int get_qp_table(AVFrame *in, int8_t **table, int *stride) +{ + AVFrameSideData *sd; + AVVideoEncParams *par; + unsigned int mb_h = (in->height + 15) / 16; + unsigned int mb_w = (in->width + 15) / 16; + unsigned int nb_mb = mb_h * mb_w; + unsigned int block_idx; + + sd = av_frame_get_side_data(in, AV_FRAME_DATA_VIDEO_ENC_PARAMS); + if (!sd) + return 0; + par = (AVVideoEncParams*)sd->data; + if (par->type != AV_VIDEO_ENC_PARAMS_MPEG2 || + (par->nb_blocks != 0 && par->nb_blocks != nb_mb)) + return 0; + + *table = av_malloc(nb_mb); + if (!*table) + return AVERROR(ENOMEM); + *stride = mb_w; + + if (par->nb_blocks == 0) { + memset(*table, par->qp, nb_mb); + return 0; + } + + for (block_idx = 0; block_idx < nb_mb; block_idx++) { + AVVideoBlockParams *b = av_video_enc_params_block(par, block_idx); + (*table)[block_idx] = par->qp + b->delta_qp; + } + + return 0; +} + static int pp_filter_frame(AVFilterLink *inlink, AVFrame *inbuf) { AVFilterContext *ctx = inlink->dst; @@ -126,8 +163,9 @@ static int pp_filter_frame(AVFilterLink *inlink, AVFrame *inbuf) const int aligned_w = FFALIGN(outlink->w, 8); const int aligned_h = FFALIGN(outlink->h, 8); AVFrame *outbuf; - int qstride, qp_type; - int8_t *qp_table ; + int qstride = 0; + int8_t *qp_table = NULL; + int ret; outbuf = ff_get_video_buffer(outlink, aligned_w, aligned_h); if (!outbuf) { @@ -137,7 +175,14 @@ static int pp_filter_frame(AVFilterLink *inlink, AVFrame *inbuf) av_frame_copy_props(outbuf, inbuf); outbuf->width = inbuf->width; outbuf->height = inbuf->height; - qp_table = av_frame_get_qp_table(inbuf, &qstride, &qp_type); + + ret = get_qp_table(inbuf, &qp_table, &qstride); + if (ret < 0) { + av_frame_free(&inbuf); + av_frame_free(&outbuf); + av_freep(&qp_table); + return ret; + } pp_postprocess((const uint8_t **)inbuf->data, inbuf->linesize, outbuf->data, outbuf->linesize, @@ -146,9 +191,10 @@ static int pp_filter_frame(AVFilterLink *inlink, AVFrame *inbuf) qstride, pp->modes[pp->mode_id], pp->pp_ctx, - outbuf->pict_type | (qp_type ? PP_PICT_TYPE_QP2 : 0)); + outbuf->pict_type | (qp_table ? PP_PICT_TYPE_QP2 : 0)); av_frame_free(&inbuf); + av_freep(&qp_table); return ff_filter_frame(outlink, outbuf); } diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak index 3d0d4969b8..cb7ce7a158 100644 --- a/tests/fate/filter-video.mak +++ b/tests/fate/filter-video.mak @@ -535,7 +535,7 @@ FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP) $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd -fate-filter-pp: CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" +fate-filter-pp: CMD = framecrc -flags bitexact -export_side_data venc_params -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al" fate-filter-pp1: CMD = video_filter "pp=fq|4/be/hb/vb/tn/l5/al" fate-filter-pp2: CMD = video_filter "qp=2*(x+y),pp=be/h1/v1/lb" fate-filter-pp3: CMD = video_filter "qp=2*(x+y),pp=be/ha|128|7/va/li"