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;
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()?
>
>
> 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.
Thanks for writing this up. Let's copy that (and link this thread) to
https://wiki.libav.org/Migration/10
--
regards,
Reinhard
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
