[FFmpeg-devel] [PATCH] avutil/frame: Add avcodec_private

Discussion:

[FFmpeg-devel] [PATCH] avutil/frame: Add avcodec_private_ref to AVFrame

Michael Niedermayer

2017-11-05 12:34:51 UTC

This gives libavcodec a field that it can freely and safely use.
Avoiding the need of wraping of a users opaque_ref field and its issues.

Signed-off-by: Michael Niedermayer <***@niedermayer.cc>
---
libavutil/frame.c | 8 +++++++-
libavutil/frame.h | 10 ++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 982fbb5c81..6ddaef1e74 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -383,12 +383,17 @@ FF_ENABLE_DEPRECATION_WARNINGS
#endif

av_buffer_unref(&dst->opaque_ref);
+ av_buffer_unref(&dst->avcodec_private_ref);
if (src->opaque_ref) {
dst->opaque_ref = av_buffer_ref(src->opaque_ref);
if (!dst->opaque_ref)
return AVERROR(ENOMEM);
}
-
+ if (src->avcodec_private_ref) {
+ dst->avcodec_private_ref = av_buffer_ref(src->avcodec_private_ref);
+ if (!dst->avcodec_private_ref)
+ return AVERROR(ENOMEM);
+ }
return 0;
}

@@ -524,6 +529,7 @@ void av_frame_unref(AVFrame *frame)
av_buffer_unref(&frame->hw_frames_ctx);

av_buffer_unref(&frame->opaque_ref);
+ av_buffer_unref(&frame->avcodec_private_ref);

get_frame_defaults(frame);
}
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 0c6aab1c02..73b7d949a9 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -563,6 +563,16 @@ typedef struct AVFrame {
/**
* @}
*/
+ /**
+ * AVBufferRef for free use by libavcodec. Code outside avcodec will never
+ * check or change the contents of the buffer ref. FFmpeg calls
+ * av_buffer_unref() on it when the frame is unreferenced.
+ * av_frame_copy_props() calls create a new reference with av_buffer_ref()
+ * for the target frame's avcodec_private_ref field.
+ *
+ * avcodec should never assign mutually incompatible types to this field.
+ */
+ AVBufferRef *avcodec_private_ref;
} AVFrame;

#if FF_API_FRAME_GET_SET

--
2.15.0

James Almer

2017-11-05 13:35:06 UTC

Permalink

Post by Michael Niedermayer
This gives libavcodec a field that it can freely and safely use.
Avoiding the need of wraping of a users opaque_ref field and its issues.

Could this perhaps be in an opaque internal struct instead, much like
AVCodecInternal and whatnot? As wm4 said in the relevant discussion,
this approach is nonoptimal and *will* snowball into a mess of fields if
other libav* libraries start requiring their own buffers in a frame.
An internal field of an opaque struct being in a public header is much
cleaner and easier to maintain than adding such specific fields that may
at some point in the future need to be removed.

No actual comments about the approach in question to solve the issue.
Will leave that to someone more knowledgeable. But at least I'm glad
something is being done about it.

Post by Michael Niedermayer
---
libavutil/frame.c | 8 +++++++-
libavutil/frame.h | 10 ++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 982fbb5c81..6ddaef1e74 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -383,12 +383,17 @@ FF_ENABLE_DEPRECATION_WARNINGS
#endif
av_buffer_unref(&dst->opaque_ref);
+ av_buffer_unref(&dst->avcodec_private_ref);
if (src->opaque_ref) {
dst->opaque_ref = av_buffer_ref(src->opaque_ref);
if (!dst->opaque_ref)
return AVERROR(ENOMEM);
}
-
+ if (src->avcodec_private_ref) {
+ dst->avcodec_private_ref = av_buffer_ref(src->avcodec_private_ref);
+ if (!dst->avcodec_private_ref)
+ return AVERROR(ENOMEM);
+ }
return 0;
}
@@ -524,6 +529,7 @@ void av_frame_unref(AVFrame *frame)
av_buffer_unref(&frame->hw_frames_ctx);
av_buffer_unref(&frame->opaque_ref);
+ av_buffer_unref(&frame->avcodec_private_ref);
get_frame_defaults(frame);
}
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 0c6aab1c02..73b7d949a9 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -563,6 +563,16 @@ typedef struct AVFrame {
/**
*/
+ /**
+ * AVBufferRef for free use by libavcodec. Code outside avcodec will never
+ * check or change the contents of the buffer ref. FFmpeg calls
+ * av_buffer_unref() on it when the frame is unreferenced.
+ * av_frame_copy_props() calls create a new reference with av_buffer_ref()
+ * for the target frame's avcodec_private_ref field.
+ *
+ * avcodec should never assign mutually incompatible types to this field.
+ */
+ AVBufferRef *avcodec_private_ref;
} AVFrame;
#if FF_API_FRAME_GET_SET

Michael Niedermayer

2017-11-05 15:20:07 UTC

Permalink

Post by James Almer

Post by Michael Niedermayer
This gives libavcodec a field that it can freely and safely use.
Avoiding the need of wraping of a users opaque_ref field and its issues.

Could this perhaps be in an opaque internal struct instead, much like
AVCodecInternal and whatnot?

We could do a AVFrameInternal but that would require some differenecs
to AVCodecInternal.

The AVBufferRef from the patch has its own reference counting and
destruction callback. (which avcodec can use for cleaning it up)

a straight AVFrameInternal (in AVCodecInternal style) would not have
that, we could place the AVFrameInternal inside a AVBufferRef or
provide seperate API / fields to make cleanup from libavcodec possible.

Post by James Almer
As wm4 said in the relevant discussion,
this approach is nonoptimal and *will* snowball into a mess of fields if
other libav* libraries start requiring their own buffers in a frame.

I remember and i think i didnt reply there but this is not really
a plausible scenario.

If we added a field per lib we would have realistically 2 fields
one for libavcodec and one for libavfilter. Beyond that even with
constructed use cases it seems to become hard (at least to me ATM) to
see what else would be added. libswscale ? libavformat ? but what would
they do with their fields ?

But really i primarly want this to move forward, i have no real
preferrance as long as we dont cram muliple opaques in a single opaque
field with wraping or other.

Post by James Almer
An internal field of an opaque struct being in a public header is much
cleaner and easier to maintain than adding such specific fields that may
at some point in the future need to be removed.
No actual comments about the approach in question to solve the issue.
Will leave that to someone more knowledgeable. But at least I'm glad
something is being done about it.

[...]

--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf

Timo Rothenpieler

2017-11-06 12:19:18 UTC

Permalink

Post by James Almer

Post by Michael Niedermayer
This gives libavcodec a field that it can freely and safely use.
Avoiding the need of wraping of a users opaque_ref field and its issues.

The problem here is that avcodec, due to nested decoders and the like,
might potentially wrap this field multiple times internally, so it
basically has to be something avcodec specific.

James Almer

2017-11-06 13:23:24 UTC

Permalink

Post by Timo Rothenpieler

Post by James Almer

Post by Michael Niedermayer
This gives libavcodec a field that it can freely and safely use.
Avoiding the need of wraping of a users opaque_ref field and its issues.

The problem here is that avcodec, due to nested decoders and the like,
might potentially wrap this field multiple times internally, so it
basically has to be something avcodec specific.

And an opaque internal struct would require avpriv_ symbols to be
accessed from outside libavutil, so it's an ugly solution nonetheless. I
already told Michael to discard the suggestion.

Hendrik Leppkes

2017-11-05 13:52:50 UTC

Permalink

On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer

Post by Michael Niedermayer
This gives libavcodec a field that it can freely and safely use.
Avoiding the need of wraping of a users opaque_ref field and its issues.
---
libavutil/frame.c | 8 +++++++-
libavutil/frame.h | 10 ++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 982fbb5c81..6ddaef1e74 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -383,12 +383,17 @@ FF_ENABLE_DEPRECATION_WARNINGS
#endif
av_buffer_unref(&dst->opaque_ref);
+ av_buffer_unref(&dst->avcodec_private_ref);
if (src->opaque_ref) {
dst->opaque_ref = av_buffer_ref(src->opaque_ref);
if (!dst->opaque_ref)
return AVERROR(ENOMEM);
}
-
+ if (src->avcodec_private_ref) {
+ dst->avcodec_private_ref = av_buffer_ref(src->avcodec_private_ref);
+ if (!dst->avcodec_private_ref)
+ return AVERROR(ENOMEM);
+ }
return 0;
}
@@ -524,6 +529,7 @@ void av_frame_unref(AVFrame *frame)
av_buffer_unref(&frame->hw_frames_ctx);
av_buffer_unref(&frame->opaque_ref);
+ av_buffer_unref(&frame->avcodec_private_ref);
get_frame_defaults(frame);
}
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 0c6aab1c02..73b7d949a9 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -563,6 +563,16 @@ typedef struct AVFrame {
/**
*/
+ /**
+ * AVBufferRef for free use by libavcodec. Code outside avcodec will never
+ * check or change the contents of the buffer ref. FFmpeg calls
+ * av_buffer_unref() on it when the frame is unreferenced.
+ * av_frame_copy_props() calls create a new reference with av_buffer_ref()
+ * for the target frame's avcodec_private_ref field.
+ *
+ * avcodec should never assign mutually incompatible types to this field.
+ */
+ AVBufferRef *avcodec_private_ref;
} AVFrame;
#if FF_API_FRAME_GET_SET

I would prefer if this field would not be library-specific, but
perhaps just "private_ref" which is not allowed to be touched by
users, and documented to only be valid while within one library - ie.
if you pass a frame from avcodec to avfilter, avfilter could take over
the field (and just free any info, if its still in there).
This would avoid any chances of adding a multitude of fields later,
and a single AVFrame instance is not going to be used in multiple
libraries at the same time anyway (the contents might, but not the
actual AVFrame struct)

- Hendrik

Michael Niedermayer

2017-11-05 15:25:53 UTC

Permalink

Post by Hendrik Leppkes
On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer

that should be easy to implement ...

a disadvantage is the slightly higher chance of mixing up types if
some codepath doesnt cleanup the field

question is what do most prefer ?
avcodec_private_ref ? (that is one for each of the 2 libs)
private_ref ?
avframe_internal_ref ? (that is a private struct defined in avutil similar to AVCodecInternal)

[...]

--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2

James Almer

2017-11-05 23:48:43 UTC

Permalink

Post by Michael Niedermayer

Post by Hendrik Leppkes
On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer

that should be easy to implement ...
a disadvantage is the slightly higher chance of mixing up types if
some codepath doesnt cleanup the field
question is what do most prefer ?
avcodec_private_ref ? (that is one for each of the 2 libs)
private_ref ?
avframe_internal_ref ? (that is a private struct defined in avutil similar to AVCodecInternal)

Discard my suggestion. Being inside an internal opaque struct would
require avpriv_ functions to access from within avcodec, and as BtbN
mentioned on IRC earlier today we should definitely avoid that.

So take Hendrik's suggestion, unless somebody starts a vote to force
wm4's original implementation instead.

Timo Rothenpieler

2017-11-06 12:23:01 UTC

Permalink

Post by Michael Niedermayer

Post by Hendrik Leppkes
I would prefer if this field would not be library-specific, but
perhaps just "private_ref" which is not allowed to be touched by
users, and documented to only be valid while within one library - ie.
if you pass a frame from avcodec to avfilter, avfilter could take over
the field (and just free any info, if its still in there).
This would avoid any chances of adding a multitude of fields later,
and a single AVFrame instance is not going to be used in multiple
libraries at the same time anyway (the contents might, but not the
actual AVFrame struct)

that should be easy to implement ...
a disadvantage is the slightly higher chance of mixing up types if
some codepath doesnt cleanup the field
question is what do most prefer ?
avcodec_private_ref ? (that is one for each of the 2 libs)
private_ref ?
avframe_internal_ref ? (that is a private struct defined in avutil similar to AVCodecInternal)

I like private_ref.

Following this approach also keeps the diff to libav small.

Rostislav Pehlivanov

2017-11-05 18:08:41 UTC

Permalink

Post by Hendrik Leppkes
On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer

private_ref);

Post by Michael Niedermayer
+ if (!dst->avcodec_private_ref)
+ return AVERROR(ENOMEM);
+ }
return 0;
}
@@ -524,6 +529,7 @@ void av_frame_unref(AVFrame *frame)
av_buffer_unref(&frame->hw_frames_ctx);
av_buffer_unref(&frame->opaque_ref);
+ av_buffer_unref(&frame->avcodec_private_ref);
get_frame_defaults(frame);
}
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 0c6aab1c02..73b7d949a9 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -563,6 +563,16 @@ typedef struct AVFrame {
/**
*/
+ /**
+ * AVBufferRef for free use by libavcodec. Code outside avcodec

will never

Post by Michael Niedermayer
+ * check or change the contents of the buffer ref. FFmpeg calls
+ * av_buffer_unref() on it when the frame is unreferenced.
+ * av_frame_copy_props() calls create a new reference with

av_buffer_ref()

Post by Michael Niedermayer
+ * for the target frame's avcodec_private_ref field.
+ *
+ * avcodec should never assign mutually incompatible types to this

field.

Post by Michael Niedermayer
+ */
+ AVBufferRef *avcodec_private_ref;
} AVFrame;
#if FF_API_FRAME_GET_SET

No, this would be horrible, lavfi should be able to use what's in the
avbufferref without reinitializing anything that it should contain. lavu
should handle anything that has to be initialized in the struct the
avbufferref backs so that all internal libraries can use it.

Hendrik Leppkes

2017-11-05 18:18:22 UTC

Permalink

On Sun, Nov 5, 2017 at 7:08 PM, Rostislav Pehlivanov

Post by Rostislav Pehlivanov

Post by Hendrik Leppkes
On Sun, Nov 5, 2017 at 1:34 PM, Michael Niedermayer

private_ref);

will never

av_buffer_ref()

Post by Michael Niedermayer
+ * for the target frame's avcodec_private_ref field.
+ *
+ * avcodec should never assign mutually incompatible types to this

field.

Post by Michael Niedermayer
+ */
+ AVBufferRef *avcodec_private_ref;
} AVFrame;
#if FF_API_FRAME_GET_SET

The entire point of this field is to contain opaque private data that
no other library ever has any business of even knowing exists.
If additional data needs to be shared between libraries, it should be
a properly defined public shared structure.

- Hendrik