#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 )
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)
Change History (9)
comment:1 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Summary: | enc_x264 needs to let us manage the buffers as we see fit → dec_avcodec needs to let us manage the buffers as we see fit |
comment:2 Changed 9 years ago by
Priority: | critical → blocker |
---|
comment:4 Changed 9 years ago by
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 9 years ago by
Reverted in r3970 - too many problems with it, see changeset for details.
Changed 9 years ago by
Attachment: | python-frame-tracking.patch added |
---|
implements frame tracking in python
comment:6 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 anAVImageWrapper
for the pixels and when that is freed, we callxpra_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 callav_free()
which callsfree()
since we had already calledxpra_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()
comment:8 Changed 16 months ago by
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/350
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".