On Sat, 11 Apr 2020, James Almer wrote:
On 4/11/2020 6:35 PM, Andreas Rheinhardt wrote:
James Almer:
On 4/11/2020 6:19 PM, Andreas Rheinhardt wrote:
Currently uncoded frames (i.e. packets whose data actually points to an
AVFrame) are not refcounted. As a consequence, calling av_packet_unref()
on them will not free them, but may simply make sure that they leak by
losing the pointer to the frame.
This commit changes this by mimicking what is being done for wrapped
AVFrames: Ownership of the AVFrame is passed to a special AVBuffer with
a custom free function that properly frees the AVFrame. The packet is
equipped with an AVBufferRef referencing this AVBuffer. Thereby the
packet becomes compatible with av_packet_unref().
This already has three advantages, all in interleaved mode:
1. If an error happens at the preparatory steps (before the packet is
put into the interleavement queue), the frame is properly freed.
2. If the trailer is never written, the frames still in the
interleavement queue will now be properly freed by
ff_packet_list_free().
3. The custom code for moving the packet to the packet list in
ff_interleave_add_packet() can be removed.
It will also simplify fixing further memleaks in future commits.
Given that the AVFrame is now owned by an AVBuffer, the muxer may no
longer take ownership of the AVFrame, because the function used to call
the muxer when writing uncoded frames does not allow to transfer
ownership of the reference contained in the packet. (Changing the
function signature is not possible (except at a major version bump),
because most of these muxers reside in libavdevice.) But this is no loss
as none of the muxers ever made use of this. This change has been
documented.
Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com>
---
The new semantic of AVOutputFormat.write_uncoded_frame() basically boils
down to treat frame as AVFrame * const *. I wonder whether I should
simply set it that way and remove the then redundant comment.
libavformat/avformat.h | 4 ++--
libavformat/mux.c | 43 ++++++++++++++++++++++++------------------
2 files changed, 27 insertions(+), 20 deletions(-)
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 39b99b4481..89207b9823 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -578,8 +578,8 @@ typedef struct AVOutputFormat {
*
* See av_write_uncoded_frame() for details.
*
- * The library will free *frame afterwards, but the muxer can prevent it
- * by setting the pointer to NULL.
+ * The muxer must not change *frame, but it can keep the content of **frame
+ * by av_frame_move_ref().
*/
int (*write_uncoded_frame)(struct AVFormatContext *, int stream_index,
AVFrame **frame, unsigned flags);
diff --git a/libavformat/mux.c b/libavformat/mux.c
index cc2d1e275a..712ba0c319 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -550,12 +550,6 @@ fail:
#define AV_PKT_FLAG_UNCODED_FRAME 0x2000
-/* Note: using sizeof(AVFrame) from outside lavu is unsafe in general, but
- it is only being used internally to this file as a consistency check.
- The value is chosen to be very unlikely to appear on its own and to cause
- immediate failure if used anywhere as a real size. */
-#define UNCODED_FRAME_PACKET_SIZE (INT_MIN / 3 * 2 + (int)sizeof(AVFrame))
-
#if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX
FF_DISABLE_DEPRECATION_WARNINGS
@@ -747,9 +741,10 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt)
if ((pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) {
AVFrame *frame = (AVFrame *)pkt->data;
- av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE);
+ av_assert0(pkt->size == sizeof(*frame));
sizeof(AVFrame) is not part of the ABI.
This is not the first case of this violation happening in lavf, so it
would be best to not make it worse.
I know. And I actually thought that I don't make it worse because
UNCODED_FRAME_PACKET_SIZE which is replaced here also uses
sizeof(AVFrame).
Ugh, yes, you're right. Guess it makes no difference, then.
Can't you simply store a pointer to an AVFrame in the data?
I did not want to set a negative size, because someone
might add a check to av_buffer_create() that errors out in this case.
(Btw: libavcodec/wrapped_avframe.c also violates this.)
Maybe wrapped_avframe should also be changed eventually to only store a
pointer to an AVFrame? That would at least fix the issue that realloc-ing
the packet data corrupts the AVFrame because of extended_data referencing
the AVFrame itself.
Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
