xpra icon
Bug tracker and wiki

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#350 closed task (fixed)

dec_avcodec needs to let us manage the buffers as we see fit

Reported by: Antoine Martin Owned by: ahuillet
Priority: blocker Milestone: 0.10
Component: server Version:
Keywords: Cc:

Description (last modified by Antoine Martin)

We need to have control over the lifecycle of the buffer as it may get used from another thread.
For an overview of the problems this causes, see r3570 - the code is very brittle because of this misfeature.

This is also true of enc_x264 (to a lesser extent).

Attachments (1)

python-frame-tracking.patch (11.2 KB) - added by Antoine Martin 6 years ago.
implements frame tracking in python

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by Antoine Martin

Description: modified (diff)
Summary: enc_x264 needs to let us manage the buffers as we see fitdec_avcodec needs to let us manage the buffers as we see fit

comment:2 Changed 6 years ago by Antoine Martin

Priority: criticalblocker

comment:3 Changed 6 years ago by ahuillet

r3955 implements a buffer tracking logic in dec_avcodec.

avcodec notifies dec_avcodec when it is done with a buffer. Instead of freeing it immediately (which is avcodec's default behavior), we now wait for *both* avcodec and xpra to be done with the buffer.

Xpra needs to call xpra_release_buffer(data[0]) to notify dec_avcodec that it is done with a given buffer. dec_avcodec will then work from there to find the AVFrame * pointer, and mark it as "xpra released".

Last edited 6 years ago by Antoine Martin (previous) (diff)

comment:4 Changed 6 years ago by ahuillet

Once the xpra_release_buffer() calls have been added, dec_avcodec's tracking logic will need to be fully enabled, with the following patch:

diff --git a/src/xpra/codecs/dec_avcodec/dec_avcodec.c b/src/xpra/codecs/dec_avcodec/dec_avcodec.c
index b36238d..4697ed7 100644
--- a/src/xpra/codecs/dec_avcodec/dec_avcodec.c
+++ b/src/xpra/codecs/dec_avcodec/dec_avcodec.c
@@ -148,7 +148,7 @@ static int my_avcodec_get_buffer(AVCodecContext *avctx, AVFrame *frame)
 static void release_buffer_if_ready(struct frame_pointer *f, AVCodecContext *avctx, AVFrame *frame)
 {
        // If buffer is released both by avcodec and xpra, free it...
-       if (f->avcodec_released) { // disable xpra tracking until hooked up && f->xpra_released) {
+       if (f->avcodec_released && f->xpra_released) {
                avcodec_default_release_buffer(avctx, frame);
        }

comment:5 Changed 6 years ago by Antoine Martin

Reverted in r3970 - too many problems with it, see changeset for details.

Changed 6 years ago by Antoine Martin

Attachment: python-frame-tracking.patch added

implements frame tracking in python

comment:6 Changed 6 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

Done in r3976, lots of details both in changelog and in the code itself since the "pointer as dictionary key" isn't obvious.


with XPRA_AVCODEC_DEBUG=1 we can see:

  • avcodec_get_buffer makes us create and register a new framewrapper, then we also create an AVImageWrapper for the pixels and when that is freed, we call xpra_free:
    add_framewrapper(0x7f79980ca430L, AVFrameWrapper(0x7f79980ca430)) known frame keys: []
    avcodec: 257856 bytes in <class 'xpra.codecs.dec_avcodec.decoder.AVImageWrapper'>(YUV420P:(0, 0, 498, 316, 24))
    AVImageWrapper.free() av_frame=AVFrameWrapper(0x7f79980ca430)
    xpra_free(0x7f79980ca430) framewrapper=AVFrameWrapper(0x7f79980ca430)
    AVFrameWrapper(0x7f79980ca430).xpra_free()
    AVImageWrapper.free() av_frame=None
    
  • next frame comes in and we do the same again
    add_framewrapper(0x7f7998174f50L, AVFrameWrapper(0x7f7998174f50)) known frame keys: ['0x7f79980ca430']
    avcodec: 257856 bytes in <class 'xpra.codecs.dec_avcodec.decoder.AVImageWrapper'>(YUV420P:(0, 0, 498, 316, 24))
    AVImageWrapper.free() av_frame=AVFrameWrapper(0x7f7998174f50)
    xpra_free(0x7f7998174f50) framewrapper=AVFrameWrapper(0x7f7998174f50)
    AVFrameWrapper(0x7f7998174f50).xpra_free()
    AVImageWrapper.free() av_frame=None
    
  • when the next frame comes in, avcodec frees the first frame (0x7f79980ca430L), so we call av_free() which calls free() since we had already called xpra_free() on this frame at step 1:
    avcodec_release_buffer(0x7f7998002160L, 0x7f79980ca430L) \
       decoder=<xpra.codecs.dec_avcodec.decoder.Decoder object at 0x1f7b590>, frame_key=140160218735664
    av_free(0x7f79980ca430L) framewrapper=AVFrameWrapper(0x7f79980ca430)
    AVFrameWrapper(0x7f79980ca430).av_free()
    AVFrameWrapper(0x7f79980ca430).free()
    

Then we continue processing the frame as before:

add_framewrapper(0x7f79980ca430L, AVFrameWrapper(0x7f79980ca430)) known frame keys: ['0x7f7998174f50']
avcodec: 257856 bytes in <class 'xpra.codecs.dec_avcodec.decoder.AVImageWrapper'>(YUV420P:(0, 0, 498, 316, 24))
AVImageWrapper.free() av_frame=AVFrameWrapper(0x7f79980ca430)
xpra_free(0x7f79980ca430) framewrapper=AVFrameWrapper(0x7f79980ca430)
AVFrameWrapper(0x7f79980ca430).xpra_free()
AVImageWrapper.free() av_frame=None

etc


On exit, we cleanup the decoder, which ends up firing avcodec_release_buffer on the 2 remaining frames it was still using and we can actually free them:

avcodec_release_buffer(0x7f7998002160L, 0x7f79980ca430L) \
    decoder=<xpra.codecs.dec_avcodec.decoder.Decoder object at 0x1f7b590>, frame_key=140160218735664
av_free(0x7f79980ca430L) framewrapper=AVFrameWrapper(0x7f79980ca430)
AVFrameWrapper(0x7f79980ca430).av_free()
AVFrameWrapper(0x7f79980ca430).free()
avcodec_release_buffer(0x7f7998002160L, 0x7f7998174f50L) \
    decoder=<xpra.codecs.dec_avcodec.decoder.Decoder object at 0x1f7b590>, frame_key=140160219434832
av_free(0x7f7998174f50L) framewrapper=AVFrameWrapper(0x7f7998174f50)
AVFrameWrapper(0x7f7998174f50).av_free()
AVFrameWrapper(0x7f7998174f50).free()
Last edited 6 years ago by Antoine Martin (previous) (diff)

comment:7 Changed 6 years ago by Antoine Martin

Important fix for the context free code in r3983

Note: See TracTickets for help on using tickets.