| Message ID | 1562944894-29396-4-git-send-email-hwrenx@126.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 12 Jul 2019, hwrenx wrote: > Can effectivly improved decoding speed when memcpy becomes a limitation > for proccessing high resolution source. > Tested under i7-8700k with `ffmpeg -i 7680x4320.avs2 -vsync 0 -f null -` > got performance 23fps => 42fps > > Signed-off-by: hwrenx <hwrenx@126.com> > --- > libavcodec/libdavs2.c | 54 +++++++++++++++++++++++++++++---------------------- > 1 file changed, 31 insertions(+), 23 deletions(-) > > diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c > index 1b274a3..af0778f 100644 > --- a/libavcodec/libdavs2.c > +++ b/libavcodec/libdavs2.c > @@ -60,13 +60,24 @@ static av_cold int davs2_init(AVCodecContext *avctx) > return 0; > } > > +static void davs2_frame_unref(void *opaque, uint8_t *data) { > + DAVS2Context *cad = (DAVS2Context *)opaque; > + davs2_picture_t pic; > + > + pic.magic = (davs2_picture_t *)data; > + > + if (cad->decoder) { > + davs2_decoder_frame_unref(cad->decoder, &pic); > + } else { > + av_log(NULL, AV_LOG_WARNING, "Decoder not found, frame unreference failed.\n"); Whoa, this should not happen, and you have to be prepared that the user might close the decoder before freeing the last frame. Maybe you should use some refcounting and create references to the decoder, others may have a better idea. Regards, Marton
At 2019-07-13 01:38:55, "Marton Balint" <cus@passwd.hu> wrote: > > >On Fri, 12 Jul 2019, hwrenx wrote: > >> Can effectivly improved decoding speed when memcpy becomes a limitation >> for proccessing high resolution source. >> Tested under i7-8700k with `ffmpeg -i 7680x4320.avs2 -vsync 0 -f null -` >> got performance 23fps => 42fps >> >> Signed-off-by: hwrenx <hwrenx@126.com> >> --- >> libavcodec/libdavs2.c | 54 +++++++++++++++++++++++++++++---------------------- >> 1 file changed, 31 insertions(+), 23 deletions(-) >> >> diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c >> index 1b274a3..af0778f 100644 >> --- a/libavcodec/libdavs2.c >> +++ b/libavcodec/libdavs2.c >> @@ -60,13 +60,24 @@ static av_cold int davs2_init(AVCodecContext *avctx) >> return 0; >> } >> >> +static void davs2_frame_unref(void *opaque, uint8_t *data) { >> + DAVS2Context *cad = (DAVS2Context *)opaque; >> + davs2_picture_t pic; >> + >> + pic.magic = (davs2_picture_t *)data; >> + >> + if (cad->decoder) { >> + davs2_decoder_frame_unref(cad->decoder, &pic); >> + } else { >> + av_log(NULL, AV_LOG_WARNING, "Decoder not found, frame unreference failed.\n"); > >Whoa, this should not happen, and you have to be prepared that the user >might close the decoder before freeing the last frame. I hope so, actually it's harmless if we failed to unref some frames outside davs2 when decoder was closed through davs2_decoder_close(). The decoder will free the memory which contains all recycle data itself. > >Maybe you should use some refcounting and create references to the >decoder, others may have a better idea. > >Regards, >Marton Of course, anyway, it would be better if there are any ways to make sure we correctly freed all frames even decoder was not found or exited without a davs2_decoder_close(). But force to free the memory immediately in the call back function can cause troubles to decoder and I'm not very clearly understand how to use reference to deal with this problem inside a codec. Maybe that means I could reference the buffer and waiting ffmpeg to free them once they were not handled by davs2? Thanks :-) Huiwen REN >_______________________________________________ >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".
On Sat, 13 Jul 2019, hwren wrote: > > At 2019-07-13 01:38:55, "Marton Balint" <cus@passwd.hu> wrote: >> >> >> On Fri, 12 Jul 2019, hwrenx wrote: >> >>> Can effectivly improved decoding speed when memcpy becomes a limitation >>> for proccessing high resolution source. >>> Tested under i7-8700k with `ffmpeg -i 7680x4320.avs2 -vsync 0 -f null -` >>> got performance 23fps => 42fps >>> >>> Signed-off-by: hwrenx <hwrenx@126.com> >>> --- >>> libavcodec/libdavs2.c | 54 +++++++++++++++++++++++++++++---------------------- >>> 1 file changed, 31 insertions(+), 23 deletions(-) >>> >>> diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c >>> index 1b274a3..af0778f 100644 >>> --- a/libavcodec/libdavs2.c >>> +++ b/libavcodec/libdavs2.c >>> @@ -60,13 +60,24 @@ static av_cold int davs2_init(AVCodecContext *avctx) >>> return 0; >>> } >>> >>> +static void davs2_frame_unref(void *opaque, uint8_t *data) { >>> + DAVS2Context *cad = (DAVS2Context *)opaque; >>> + davs2_picture_t pic; >>> + >>> + pic.magic = (davs2_picture_t *)data; >>> + >>> + if (cad->decoder) { >>> + davs2_decoder_frame_unref(cad->decoder, &pic); >>> + } else { >>> + av_log(NULL, AV_LOG_WARNING, "Decoder not found, frame unreference failed.\n"); >> >> Whoa, this should not happen, and you have to be prepared that the user >> might close the decoder before freeing the last frame. > > I hope so, actually it's harmless if we failed to unref some frames outside davs2 when decoder > was closed through davs2_decoder_close(). The decoder will free the memory which contains > all recycle data itself. And that is the problem, because the data should survive closing the (avcodec) decoder. > >> >> Maybe you should use some refcounting and create references to the >> decoder, others may have a better idea. >> >> Regards, >> Marton > > Of course, anyway, it would be better if there are any ways to make sure we correctly freed all > frames even decoder was not found or exited without a davs2_decoder_close(). But force to > free the memory immediately in the call back function can cause troubles to decoder and I'm > not very clearly understand how to use reference to deal with this problem inside a codec. > > > Maybe that means I could reference the buffer and waiting ffmpeg to free them once they were > not handled by davs2? I am not sure I understand your concern here. Here is what I propose in more detail, I think it works for all cases: In davs_init allocate a small struct which contains the decoder and a reference counter for the decoder itself: DAVS2Reference { atomic_int refcount; void *decoder; } set the refs to 1. In davs2_dump_frames you should pass the allocated DAVS2Reference to av_buffer_create instead of the DAVS2Context. After each successful av_buffer_create you should increase refcount. In davs2_frame_unref after freeing the frame you should decrease the reference counter and if you reach 0 then you can destroy the davs2 decoder (and free DAVS2Reference struct as well) You can do the same in davs2_end: decrease the refcount and if you reach 0 then you can close the davs2 decoder and free the DAVS2Reference struct. Regards, Marton
I'm sorry I almost missed this email. Thanks for your review. At 2019-07-23 03:42:20, "Marton Balint" <cus@passwd.hu> wrote: > > >On Sat, 13 Jul 2019, hwren wrote: > >> >> At 2019-07-13 01:38:55, "Marton Balint" <cus@passwd.hu> wrote: >>> >>> >>> On Fri, 12 Jul 2019, hwrenx wrote: >>> >>>> Can effectivly improved decoding speed when memcpy becomes a limitation >>>> for proccessing high resolution source. >>>> Tested under i7-8700k with `ffmpeg -i 7680x4320.avs2 -vsync 0 -f null -` >>>> got performance 23fps => 42fps >>>> >>>> Signed-off-by: hwrenx <hwrenx@126.com> >>>> --- >>>> libavcodec/libdavs2.c | 54 +++++++++++++++++++++++++++++---------------------- >>>> 1 file changed, 31 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c >>>> index 1b274a3..af0778f 100644 >>>> --- a/libavcodec/libdavs2.c >>>> +++ b/libavcodec/libdavs2.c >>>> @@ -60,13 +60,24 @@ static av_cold int davs2_init(AVCodecContext *avctx) >>>> return 0; >>>> } >>>> >>>> +static void davs2_frame_unref(void *opaque, uint8_t *data) { >>>> + DAVS2Context *cad = (DAVS2Context *)opaque; >>>> + davs2_picture_t pic; >>>> + >>>> + pic.magic = (davs2_picture_t *)data; >>>> + >>>> + if (cad->decoder) { >>>> + davs2_decoder_frame_unref(cad->decoder, &pic); >>>> + } else { >>>> + av_log(NULL, AV_LOG_WARNING, "Decoder not found, frame unreference failed.\n"); >>> >>> Whoa, this should not happen, and you have to be prepared that the user >>> might close the decoder before freeing the last frame. >> >> I hope so, actually it's harmless if we failed to unref some frames outside davs2 when decoder >> was closed through davs2_decoder_close(). The decoder will free the memory which contains >> all recycle data itself. > >And that is the problem, because the data should survive closing the >(avcodec) decoder. Do you mean after decoder closing, there may still some data left which should be unref by decoder ... The davs2_decoder_frame_unref() can only notice the decoder that it could reuse this part of memory, but the decoder won't free or realloc any memory. All these memory will be free together when decoder was closing. So the "survived data" in ffmpeg should just be some unuseful pointers. Or you mean even decoder is closed, there are still some data needed by ffmpeg? > >> >>> >>> Maybe you should use some refcounting and create references to the >>> decoder, others may have a better idea. >>> >>> Regards, >>> Marton >> >> Of course, anyway, it would be better if there are any ways to make sure we correctly freed all >> frames even decoder was not found or exited without a davs2_decoder_close(). But force to >> free the memory immediately in the call back function can cause troubles to decoder and I'm >> not very clearly understand how to use reference to deal with this problem inside a codec. >> >> >> Maybe that means I could reference the buffer and waiting ffmpeg to free them once they were >> not handled by davs2? > >I am not sure I understand your concern here. > >Here is what I propose in more detail, I think it works for all cases: > >In davs_init allocate a small struct which contains the decoder and a >reference counter for the decoder itself: >DAVS2Reference { > atomic_int refcount; > void *decoder; >} >set the refs to 1. > >In davs2_dump_frames you should pass the allocated DAVS2Reference to >av_buffer_create instead of the DAVS2Context. After each successful >av_buffer_create you should increase refcount. > >In davs2_frame_unref after freeing the frame you should decrease the >reference counter and if you reach 0 then you can destroy the davs2 >decoder (and free DAVS2Reference struct as well) > >You can do the same in davs2_end: decrease the refcount and if you reach 0 >then you can close the davs2 decoder and free the DAVS2Reference struct. > In my understanding, once closing decoder, ffmpeg should finish or stop outputing (not so sure, if not please correct me). So I think normally the refcount should always be zero when using decoder_close(), if not zero, maybe decoder can do nothing but wait (and never stop). If after closing decoder there are still some data needed by ffmpeg, a refcounter is necessary indeed. Regards, Huiwen REN >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-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c index 1b274a3..af0778f 100644 --- a/libavcodec/libdavs2.c +++ b/libavcodec/libdavs2.c @@ -60,13 +60,24 @@ static av_cold int davs2_init(AVCodecContext *avctx) return 0; } +static void davs2_frame_unref(void *opaque, uint8_t *data) { + DAVS2Context *cad = (DAVS2Context *)opaque; + davs2_picture_t pic; + + pic.magic = (davs2_picture_t *)data; + + if (cad->decoder) { + davs2_decoder_frame_unref(cad->decoder, &pic); + } else { + av_log(NULL, AV_LOG_WARNING, "Decoder not found, frame unreference failed.\n"); + } +} + static int davs2_dump_frames(AVCodecContext *avctx, davs2_picture_t *pic, int *got_frame, davs2_seq_info_t *headerset, int ret_type, AVFrame *frame) { DAVS2Context *cad = avctx->priv_data; - int bytes_per_sample = pic->bytes_per_sample; - int plane = 0; - int line = 0; + int plane; if (!headerset) { *got_frame = 0; @@ -104,29 +115,28 @@ static int davs2_dump_frames(AVCodecContext *avctx, davs2_picture_t *pic, int *g return AVERROR_EXTERNAL; } - for (plane = 0; plane < 3; ++plane) { - int size_line = pic->widths[plane] * bytes_per_sample; - frame->buf[plane] = av_buffer_alloc(size_line * pic->lines[plane]); - - if (!frame->buf[plane]){ - av_log(avctx, AV_LOG_ERROR, "Decoder error: allocation failure, can't dump frames.\n"); - return AVERROR(ENOMEM); - } - - frame->data[plane] = frame->buf[plane]->data; - frame->linesize[plane] = size_line; - - for (line = 0; line < pic->lines[plane]; ++line) - memcpy(frame->data[plane] + line * size_line, - pic->planes[plane] + line * pic->strides[plane], - pic->widths[plane] * bytes_per_sample); - } - frame->width = cad->headerset.width; frame->height = cad->headerset.height; frame->pts = cad->out_frame.pts; frame->format = avctx->pix_fmt; + /* handle the actual picture in magic */ + frame->buf[0] = av_buffer_create((uint8_t *)pic->magic, + sizeof(davs2_picture_t *), + davs2_frame_unref, + (void *)cad, + AV_BUFFER_FLAG_READONLY); + if (!frame->buf[0]) { + av_log(avctx, AV_LOG_ERROR, + "Decoder error: allocation failure, can't dump frames.\n"); + return AVERROR(ENOMEM); + } + + for (plane = 0; plane < 3; ++plane) { + frame->linesize[plane] = pic->strides[plane]; + frame->data[plane] = pic->planes[plane]; + } + *got_frame = 1; return 0; } @@ -158,7 +168,6 @@ static int send_delayed_frame(AVCodecContext *avctx, AVFrame *frame, int *got_fr } if (ret == DAVS2_GOT_FRAME) { ret = davs2_dump_frames(avctx, &cad->out_frame, got_frame, &cad->headerset, ret, frame); - davs2_decoder_frame_unref(cad->decoder, &cad->out_frame); } return ret; } @@ -207,7 +216,6 @@ static int davs2_decode_frame(AVCodecContext *avctx, void *data, if (ret != DAVS2_DEFAULT) { ret = davs2_dump_frames(avctx, &cad->out_frame, got_frame, &cad->headerset, ret, frame); - davs2_decoder_frame_unref(cad->decoder, &cad->out_frame); } return ret == 0 ? buf_size : ret;
Can effectivly improved decoding speed when memcpy becomes a limitation for proccessing high resolution source. Tested under i7-8700k with `ffmpeg -i 7680x4320.avs2 -vsync 0 -f null -` got performance 23fps => 42fps Signed-off-by: hwrenx <hwrenx@126.com> --- libavcodec/libdavs2.c | 54 +++++++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 23 deletions(-)