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.
The most important reasons why this API change was needed were:
- there was NO WAY WHATSOEVER for the caller to know how large does the output
buffer need to be; people usually just guessed random numbers that were either
ridiculously large or encoding randomly failed because the buffer was too
small
- the old code provided no timestamps. Note that this case, where the caller is
only interested in the raw data, is quite obscure. In the vast majority of
cases the caller will want to mux the data in some container. For that, you
need timestamps, and possibly other information, which the old API simply does
not give you. Muxers then have to resort to guessing, usually with horrible
results.
- also, if you are muxing with lavf, you need your encoded data wrapped
in an AVPacket, which the new API already does for you. And since the packet
is properly allocated, you avoid a copy, which can be a measurable performance
boost.
Due to the above, the code using the old API tended to be fragile and
error-prone, so I do not think we'll be improving the situation by restoring it.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
