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
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
