On Mon, Feb 24, 2014 at 5:43 AM, Anton Khirnov <[email protected]> wrote:
>
> On Sun, 23 Feb 2014 10:59:22 -0500, Reinhard Tartler <[email protected]>
> wrote:
>> On Sun, Feb 23, 2014 at 10:29 AM, Anton Khirnov <[email protected]> wrote:
>> >
>> > On Sun, 23 Feb 2014 09:34:46 -0500, Reinhard Tartler <[email protected]>
>> > 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
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
