diff mbox series

[FFmpeg-devel,1/3,v2] avcodec: add an AVCodecContext field to signal types of packet, frame, and coded stream side data to export

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

James Almer Feb. 10, 2020, 6:26 p.m. UTC
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(-)

Comments

Michael Niedermayer Feb. 10, 2020, 10:14 p.m. UTC | #1
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


[...]
James Almer Feb. 10, 2020, 10:40 p.m. UTC | #2
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".
>
Michael Niedermayer Feb. 11, 2020, 1:06 p.m. UTC | #3
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

[...]
James Almer Feb. 11, 2020, 1:19 p.m. UTC | #4
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.
Moritz Barsnick Feb. 11, 2020, 1:28 p.m. UTC | #5
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
James Almer Feb. 11, 2020, 1:36 p.m. UTC | #6
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.
Michael Niedermayer Feb. 11, 2020, 3:07 p.m. UTC | #7
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

[...]
James Almer Feb. 13, 2020, 7:42 p.m. UTC | #8
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).
Anton Khirnov Feb. 19, 2020, 1:36 p.m. UTC | #9
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 mbox series

Patch

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