| Message ID | 20200210182646.17593-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/3,v2] avcodec: add an AVCodecContext field to signal types of packet, frame, and coded stream side data to export | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Mon, Feb 10, 2020 at 03:26:44PM -0300, James Almer wrote: > Add an initial mvs flag to is, analog to the export_mvs flags2 one. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.h | 18 ++++++++++++++++++ > libavcodec/mpegpicture.c | 2 +- > libavcodec/mpegutils.c | 2 +- > libavcodec/options_table.h | 2 ++ > libavcodec/snowdec.c | 2 +- > libavcodec/utils.c | 3 +++ > tests/ref/fate/api-mjpeg-codec-param | 2 ++ > tests/ref/fate/api-png-codec-param | 2 ++ > 8 files changed, 30 insertions(+), 3 deletions(-) Breaks ./ffplay -debug 16384 -flags2 +export_mvs -i panda.mp4 -vf codecview=mv=pf+bf+bb [...]
On 2/10/2020 7:14 PM, Michael Niedermayer wrote: > On Mon, Feb 10, 2020 at 03:26:44PM -0300, James Almer wrote: >> Add an initial mvs flag to is, analog to the export_mvs flags2 one. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.h | 18 ++++++++++++++++++ >> libavcodec/mpegpicture.c | 2 +- >> libavcodec/mpegutils.c | 2 +- >> libavcodec/options_table.h | 2 ++ >> libavcodec/snowdec.c | 2 +- >> libavcodec/utils.c | 3 +++ >> tests/ref/fate/api-mjpeg-codec-param | 2 ++ >> tests/ref/fate/api-png-codec-param | 2 ++ >> 8 files changed, 30 insertions(+), 3 deletions(-) > > Breaks > ./ffplay -debug 16384 -flags2 +export_mvs -i panda.mp4 -vf codecview=mv=pf+bf+bb Broken how? Is there a module that's copying the decoder's AVCodeContext field that may not be copying export_side_data that i should be aware of? Or never calling avcodec_open2()? > > > [...] > > > _______________________________________________ > 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". >
On Mon, Feb 10, 2020 at 07:40:39PM -0300, James Almer wrote: > On 2/10/2020 7:14 PM, Michael Niedermayer wrote: > > On Mon, Feb 10, 2020 at 03:26:44PM -0300, James Almer wrote: > >> Add an initial mvs flag to is, analog to the export_mvs flags2 one. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavcodec/avcodec.h | 18 ++++++++++++++++++ > >> libavcodec/mpegpicture.c | 2 +- > >> libavcodec/mpegutils.c | 2 +- > >> libavcodec/options_table.h | 2 ++ > >> libavcodec/snowdec.c | 2 +- > >> libavcodec/utils.c | 3 +++ > >> tests/ref/fate/api-mjpeg-codec-param | 2 ++ > >> tests/ref/fate/api-png-codec-param | 2 ++ > >> 8 files changed, 30 insertions(+), 3 deletions(-) > > > > Breaks > > ./ffplay -debug 16384 -flags2 +export_mvs -i panda.mp4 -vf codecview=mv=pf+bf+bb > > Broken how? The filter no longer shows any motion vectors after this patch Thanks [...]
On 2/11/2020 10:06 AM, Michael Niedermayer wrote: > On Mon, Feb 10, 2020 at 07:40:39PM -0300, James Almer wrote: >> On 2/10/2020 7:14 PM, Michael Niedermayer wrote: >>> On Mon, Feb 10, 2020 at 03:26:44PM -0300, James Almer wrote: >>>> Add an initial mvs flag to is, analog to the export_mvs flags2 one. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/avcodec.h | 18 ++++++++++++++++++ >>>> libavcodec/mpegpicture.c | 2 +- >>>> libavcodec/mpegutils.c | 2 +- >>>> libavcodec/options_table.h | 2 ++ >>>> libavcodec/snowdec.c | 2 +- >>>> libavcodec/utils.c | 3 +++ >>>> tests/ref/fate/api-mjpeg-codec-param | 2 ++ >>>> tests/ref/fate/api-png-codec-param | 2 ++ >>>> 8 files changed, 30 insertions(+), 3 deletions(-) >>> >>> Breaks >>> ./ffplay -debug 16384 -flags2 +export_mvs -i panda.mp4 -vf codecview=mv=pf+bf+bb >> >> Broken how? > > The filter no longer shows any motion vectors after this patch > > Thanks What codec does that file use? I see some modules copying flags2 from one avctx to another (pthread_frame, tdsc, tiff, imm5), so i'm inclined to think this may be an issue with keeping the avctx user values synced between threads. Does the following change fix it? > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index 36ac0ac1e5..b5bd494474 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -334,6 +334,7 @@ static int update_context_from_user(AVCodecContext *dst, AVCodecContext *src) > > dst->slice_flags = src->slice_flags; > dst->flags2 = src->flags2; > + dst->export_side_data = src->export_side_data; > > copy_fields(skip_loop_filter, subtitle_header); If not, i could use that sample.
On Tue, Feb 11, 2020 at 10:19:55 -0300, James Almer wrote:
> If not, i could use that sample.
It's probably this one:
https://samples.ffmpeg.org/mov/mp4/panda.mp4
But it would be nice if Michael pointed that out. ;-)
Cheers,
Moritz
On 2/11/2020 10:28 AM, Moritz Barsnick wrote: > On Tue, Feb 11, 2020 at 10:19:55 -0300, James Almer wrote: >> If not, i could use that sample. > > It's probably this one: > https://samples.ffmpeg.org/mov/mp4/panda.mp4 > > But it would be nice if Michael pointed that out. ;-) > > Cheers, > Moritz Thanks. Looks like yeah, that fixes it, so i was correct it was threading related. But I'll wait for Michael to confirm it. Also, having all these variations of avctx copying/syncing across the codebase is kinda annoying.
On Tue, Feb 11, 2020 at 10:36:20AM -0300, James Almer wrote: > On 2/11/2020 10:28 AM, Moritz Barsnick wrote: > > On Tue, Feb 11, 2020 at 10:19:55 -0300, James Almer wrote: > >> If not, i could use that sample. > > > > It's probably this one: > > https://samples.ffmpeg.org/mov/mp4/panda.mp4 > > > > But it would be nice if Michael pointed that out. ;-) yes, would have maybe done that had my ISP worked when i wrote the mail but it didnt so i could test and write but not check where that file came from. When the ISP was up again i totally didnt think about that and just sent the mail > > > > Cheers, > > Moritz > > Thanks. Looks like yeah, that fixes it, so i was correct it was > threading related. But I'll wait for Michael to confirm it. confirmed, yout added line fixes it > > Also, having all these variations of avctx copying/syncing across the > codebase is kinda annoying. some code factorization of these would probably make this less error prone Thanks [...]
On 2/10/2020 3:26 PM, James Almer wrote: > Add an initial mvs flag to is, analog to the export_mvs flags2 one. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.h | 18 ++++++++++++++++++ > libavcodec/mpegpicture.c | 2 +- > libavcodec/mpegutils.c | 2 +- > libavcodec/options_table.h | 2 ++ > libavcodec/snowdec.c | 2 +- > libavcodec/utils.c | 3 +++ > tests/ref/fate/api-mjpeg-codec-param | 2 ++ > tests/ref/fate/api-png-codec-param | 2 ++ > 8 files changed, 30 insertions(+), 3 deletions(-) No opinions about the field or constant names? An alternative is to add one field per type of side data instead of dumping them all in one. So packet, frame and coded stream. The latter would be added once it's needed, or added now and the CPB properties side data could be made optional through it (although i don't like disabling it by default when it's been enabled for a long time now).
Quoting James Almer (2020-02-13 20:42:48) > On 2/10/2020 3:26 PM, James Almer wrote: > > Add an initial mvs flag to is, analog to the export_mvs flags2 one. > > > > Signed-off-by: James Almer <jamrial@gmail.com> > > --- > > libavcodec/avcodec.h | 18 ++++++++++++++++++ > > libavcodec/mpegpicture.c | 2 +- > > libavcodec/mpegutils.c | 2 +- > > libavcodec/options_table.h | 2 ++ > > libavcodec/snowdec.c | 2 +- > > libavcodec/utils.c | 3 +++ > > tests/ref/fate/api-mjpeg-codec-param | 2 ++ > > tests/ref/fate/api-png-codec-param | 2 ++ > > 8 files changed, 30 insertions(+), 3 deletions(-) > > No opinions about the field or constant names? > > An alternative is to add one field per type of side data instead of > dumping them all in one. So packet, frame and coded stream. The latter > would be added once it's needed, or added now and the CPB properties > side data could be made optional through it (although i don't like > disabling it by default when it's been enabled for a long time now). That seems like overkill to me. The set looks good as it is.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 77eb890549..1280a7ffe2 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -1099,6 +1099,14 @@ typedef struct RcOverride{ */ #define AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE (1 << 20) +/* Exported side data. + These flags can be passed in AVCodecContext.export_side_data before initialization. +*/ +/** + * Export motion vectors through frame side data + */ +#define AV_CODEC_EXPORT_DATA_MVS (1 << 0) + /** * Pan Scan area. * This specifies the area which should be displayed. @@ -3396,6 +3404,16 @@ typedef struct AVCodecContext { * - encoding: set by user */ int64_t max_samples; + + /** + * Bit set of AV_CODEC_EXPORT_DATA_* flags, which affects the kind of + * metadata exported in frame, packet, or coded stream side data by + * decoders and encoders. + * + * - decoding: set by user + * - encoding: set by user + */ + int export_side_data; } AVCodecContext; #if FF_API_CODEC_GET_SET diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c index ecbd77d50e..5fce25ec6e 100644 --- a/libavcodec/mpegpicture.c +++ b/libavcodec/mpegpicture.c @@ -211,7 +211,7 @@ static int alloc_picture_tables(AVCodecContext *avctx, Picture *pic, int encodin #if FF_API_DEBUG_MV avctx->debug_mv || #endif - (avctx->flags2 & AV_CODEC_FLAG2_EXPORT_MVS)) { + (avctx->export_side_data & AV_CODEC_EXPORT_DATA_MVS)) { int mv_size = 2 * (b8_array_size + 4) * sizeof(int16_t); int ref_index_size = 4 * mb_array_size; diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c index 3f94540616..c0ee3aae85 100644 --- a/libavcodec/mpegutils.c +++ b/libavcodec/mpegutils.c @@ -105,7 +105,7 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ int *low_delay, int mb_width, int mb_height, int mb_stride, int quarter_sample) { - if ((avctx->flags2 & AV_CODEC_FLAG2_EXPORT_MVS) && mbtype_table && motion_val[0]) { + if ((avctx->export_side_data & AV_CODEC_EXPORT_DATA_MVS) && mbtype_table && motion_val[0]) { const int shift = 1 + quarter_sample; const int scale = 1 << shift; const int mv_sample_log2 = avctx->codec_id == AV_CODEC_ID_H264 || avctx->codec_id == AV_CODEC_ID_SVQ3 ? 2 : 1; diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h index d4c0cdeb48..3f278d5c68 100644 --- a/libavcodec/options_table.h +++ b/libavcodec/options_table.h @@ -78,6 +78,8 @@ static const AVOption avcodec_options[] = { {"export_mvs", "export motion vectors through frame side data", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_EXPORT_MVS}, INT_MIN, INT_MAX, V|D, "flags2"}, {"skip_manual", "do not skip samples and export skip information as frame side data", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_SKIP_MANUAL}, INT_MIN, INT_MAX, A|D, "flags2"}, {"ass_ro_flush_noop", "do not reset ASS ReadOrder field on flush", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_RO_FLUSH_NOOP}, INT_MIN, INT_MAX, S|D, "flags2"}, +{"export_side_data", "Export metadata as side data", OFFSET(export_side_data), AV_OPT_TYPE_FLAGS, {.i64 = DEFAULT}, 0, UINT_MAX, A|V|S|D|E, "export_side_data"}, +{"mvs", "export motion vectors through frame side data", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_EXPORT_DATA_MVS}, INT_MIN, INT_MAX, V|D, "export_side_data"}, {"time_base", NULL, OFFSET(time_base), AV_OPT_TYPE_RATIONAL, {.dbl = 0}, 0, INT_MAX}, {"g", "set the group of picture (GOP) size", OFFSET(gop_size), AV_OPT_TYPE_INT, {.i64 = 12 }, INT_MIN, INT_MAX, V|E}, {"ar", "set audio sampling rate (in Hz)", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.i64 = DEFAULT }, 0, INT_MAX, A|D|E}, diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c index 59bd24e881..519e377a11 100644 --- a/libavcodec/snowdec.c +++ b/libavcodec/snowdec.c @@ -502,7 +502,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, ); av_assert0(!s->avmv); - if (s->avctx->flags2 & AV_CODEC_FLAG2_EXPORT_MVS) { + if (s->avctx->export_side_data & AV_CODEC_EXPORT_DATA_MVS) { s->avmv = av_malloc_array(s->b_width * s->b_height, sizeof(AVMotionVector) << (s->block_max_depth*2)); } s->avmv_index = 0; diff --git a/libavcodec/utils.c b/libavcodec/utils.c index c685b9c9d7..66041151fa 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -947,6 +947,9 @@ FF_ENABLE_DEPRECATION_WARNINGS && avctx->codec_descriptor->type == AVMEDIA_TYPE_VIDEO) av_log(avctx, AV_LOG_WARNING, "gray decoding requested but not enabled at configuration time\n"); + if (avctx->flags2 & AV_CODEC_FLAG2_EXPORT_MVS) { + avctx->export_side_data |= AV_CODEC_EXPORT_DATA_MVS; + } if ( avctx->codec->init && (!(avctx->active_thread_type&FF_THREAD_FRAME) || avci->frame_thread_encoder)) { diff --git a/tests/ref/fate/api-mjpeg-codec-param b/tests/ref/fate/api-mjpeg-codec-param index e55cef0eb9..82e3313aa9 100644 --- a/tests/ref/fate/api-mjpeg-codec-param +++ b/tests/ref/fate/api-mjpeg-codec-param @@ -4,6 +4,7 @@ stream=0, decode=0 bt=4000000 flags=0x00000000 flags2=0x00000000 + export_side_data=0x00000000 time_base=0/1 g=12 ar=0 @@ -146,6 +147,7 @@ stream=0, decode=1 bt=4000000 flags=0x00000000 flags2=0x00000000 + export_side_data=0x00000000 time_base=0/1 g=12 ar=0 diff --git a/tests/ref/fate/api-png-codec-param b/tests/ref/fate/api-png-codec-param index c04c8cc7c1..7adaa5260d 100644 --- a/tests/ref/fate/api-png-codec-param +++ b/tests/ref/fate/api-png-codec-param @@ -4,6 +4,7 @@ stream=0, decode=0 bt=4000000 flags=0x00000000 flags2=0x00000000 + export_side_data=0x00000000 time_base=0/1 g=12 ar=0 @@ -146,6 +147,7 @@ stream=0, decode=1 bt=4000000 flags=0x00000000 flags2=0x00000000 + export_side_data=0x00000000 time_base=0/1 g=12 ar=0
Add an initial mvs flag to is, analog to the export_mvs flags2 one. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.h | 18 ++++++++++++++++++ libavcodec/mpegpicture.c | 2 +- libavcodec/mpegutils.c | 2 +- libavcodec/options_table.h | 2 ++ libavcodec/snowdec.c | 2 +- libavcodec/utils.c | 3 +++ tests/ref/fate/api-mjpeg-codec-param | 2 ++ tests/ref/fate/api-png-codec-param | 2 ++ 8 files changed, 30 insertions(+), 3 deletions(-)