On Mon, Feb 24, 2014 at 5:43 AM, Anton Khirnov <an...@khirnov.net> wrote: > > On Sun, 23 Feb 2014 10:59:22 -0500, Reinhard Tartler <siret...@gmail.com> > wrote: >> On Sun, Feb 23, 2014 at 10:29 AM, Anton Khirnov <an...@khirnov.net> wrote: >> > >> > On Sun, 23 Feb 2014 09:34:46 -0500, Reinhard Tartler <siret...@gmail.com> >> > wrote: >> >> The patch quoted below requires quite non-trivial changes to >> >> applications. Consider amide's http://amide.sourceforge.net/ >> >> mpeg_encode_frame() function: >> >> >> >> >> >> gboolean mpeg_encode_frame(gpointer data, GdkPixbuf * pixbuf) { >> >> encode_t * encode = data; >> >> gint out_size; >> >> >> >> convert_rgb_pixbuf_to_yuv(encode->yuv, pixbuf); >> >> >> >> /* encode the image */ >> >> out_size = avcodec_encode_video(encode->context, >> >> encode->output_buffer, encode->output_buffer_size, encode->picture); >> >> fwrite(encode->output_buffer, 1, out_size, encode->output_file); >> >> >> >> return TRUE; >> >> }; >> >> >> >> This application so far has successfully used libavcodec without the >> >> use of AVPackets. Can someone help with explaining to amide developers >> >> why exactly their code improves by using of avcodec_encode_video2 >> >> instead? I'm not even sure if I got this patch right: >> >> >> >> >> >> --- amide-1.0.4.orig/src/mpeg_encode.c >> >> +++ amide-1.0.4/src/mpeg_encode.c >> >> @@ -268,7 +269,7 @@ gpointer mpeg_encode_setup(gchar * outpu >> >> return NULL; >> >> } >> >> >> >> - encode->picture= avcodec_alloc_frame(); >> >> + encode->picture= av_frame_alloc(); >> >> if (!encode->picture) { >> >> g_warning("couldn't allocate memory for encode->picture"); >> >> encode_free(encode); >> >> @@ -360,14 +361,37 @@ gpointer mpeg_encode_setup(gchar * outpu >> >> gboolean mpeg_encode_frame(gpointer data, GdkPixbuf * pixbuf) { >> >> encode_t * encode = data; >> >> gint out_size; >> >> + AVPacket pkt; >> >> + int ret, got_packet = 0; >> >> >> >> convert_rgb_pixbuf_to_yuv(encode->yuv, pixbuf); >> >> >> >> - /* encode the image */ >> >> - out_size = avcodec_encode_video(encode->context, >> >> encode->output_buffer, encode->output_buffer_size, encode->picture); >> >> - fwrite(encode->output_buffer, 1, out_size, encode->output_file); >> >> + av_init_packet(&pkt); >> >> + pkt.data = encode->output_buffer; >> >> + pkt.size = encode->output_buffer_size; >> > >> > No need for this, just initialize the whole packet to 0, then the encoder >> > will >> > allocate the memory for you. You can the delete output_buffer(_size) and >> > the >> > code allocating and freeing it. >> > >> >> >> >> - return TRUE; >> >> + /* encode the image */ >> >> + ret = avcodec_encode_video2(encode->context, &pkt, encode->picture, >> >> &got_packet); >> >> + if (!ret && got_packet && encode->context->coded_frame) { >> >> + encode->context->coded_frame->pts = pkt.pts; >> >> + encode->context->coded_frame->key_frame = !!(pkt.flags & >> >> AV_PKT_FLAG_KEY); >> >> + } >> > >> > The if() block is not needed. >> > >> >> + >> >> + /* free any side data since we cannot return it */ >> >> + if (pkt.side_data_elems > 0) { >> >> + int i; >> >> + for (i = 0; i < pkt.side_data_elems; i++) >> >> + av_free(pkt.side_data[i].data); >> >> + av_freep(&pkt.side_data); >> >> + pkt.side_data_elems = 0; >> >> + } >> >> + >> >> + if (!ret) { >> >> + fwrite(encode->output_buffer, 1, pkt.size, encode->output_file); >> >> + return TRUE; >> >> + } else { >> >> + return FALSE; >> >> + } >> > >> > This can be replaced by >> > if (ret >= 0 && got_packet) { >> > fwrite(pkt.data, 1, pkt.size, encode->output_file); >> > av_free_packet(&pkt); >> > } >> > return (ret >= 0) ? TRUE : FALSE; >> > >> > With those changes, the new code is about the same length as the old, >> > possibly >> > even shorter. >> >> Please double check if I got your suggestions right: >> >> >> gboolean mpeg_encode_frame(gpointer data, GdkPixbuf * pixbuf) { >> encode_t * encode = data; >> gint out_size; > > This variable is unused now > >> AVPacket pkt = { 0 }; >> int ret, got_packet = 0; >> >> convert_rgb_pixbuf_to_yuv(encode->yuv, pixbuf); >> >> /* encode the image */ >> ret = avcodec_encode_video2(encode->context, &pkt, encode->picture, >> &got_packet); >> >> if (ret >= 0 && got_packet) { >> fwrite(pkt.data, 1, pkt.size, encode->output_file); >> av_free_packet(&pkt); >> } >> return (ret >= 0) ? TRUE : FALSE; >> }; >> >> I'm not sure about the initialization part, did you really mean that >> it is not necessary to call av_init_packet()? > > Otherwise the code looks correct to me. > And yes, it's not necessary to call av_init_packet(), avcodec_encode_video2() > will do that for you. The only fields that matter are the values of packet > data/size
Thank you a lot for the review, I've updated the debian BTS with the new patch. Also now I notice that this is actually already explained in the section "Audio and video encoding APIs" of the migration document. Thanks for writing it, I hope it will help me and others in the future. -- regards, Reinhard _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel