On Sun, 18 May 2014 14:35:28 +0200, Alessandro Ghedini <[email protected]> wrote: > On Sat, May 17, 2014 at 05:35:14PM +0200, Anton Khirnov wrote: > > > > On Wed, 14 May 2014 15:56:17 +0200, Alessandro Ghedini > > <[email protected]> wrote: > > > On lun, mag 12, 2014 at 07:31:13 +0200, Anton Khirnov wrote: > > > > > > > > On Wed, 7 May 2014 00:37:41 +0200, Alessandro Ghedini > > > > <[email protected]> wrote: > > > > > Hi, > > > > > > > > > > I've been working on merging the GIF encoder from ffmpeg, since > > > > > libav's doesn't > > > > > seem to work very well (in my tests anyway), and my initial approach > > > > > has been to > > > > > simply copy/paste lavc/gif.{c,h} and lavf/gif.c, make them compile > > > > > and see how > > > > > it goes (I'm using libav.git). > > > > > > > > > > While I've been able to compile the whole thing (with a little bit of > > > > > work, due > > > > > to missing av_copy_packet() and ff_alloc_packet2()), I'm still having > > > > > problems > > > > > apparently related to pixel formats. > > > > > > > > > > Basically, when I'm using ffmpeg (ffmpeg -i test.mkv test.ffmpeg.gif) > > > > > it selects > > > > > "bgr8" and everything goes fine. > > > > > > > > > > > > > I think using av_packet_ref() really should work, there seems to be no > > > > reason to > > > > copy it. Can you post the code that fails somewhere? > > > > > > So, av_copy_packet() looks like this: > > > > > > *dst = *src; > > > return copy_packet_data(dst, src, 0); > > > > > > where copy_data_packet() does the av_buffer_ref() thing, and copies the > > > side > > > data. So I simply replaced copy_packet_data() with av_packet_ref() and > > > then used > > > av_packet_unref() instead of av_free_packet()/av_freep(), like this: > > > > > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > > > index c3787e1..c4a0d32 100644 > > > --- a/libavcodec/avpacket.c > > > +++ b/libavcodec/avpacket.c > > > @@ -262,7 +262,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > > > int av_copy_packet(AVPacket *dst, const AVPacket *src) > > > { > > > *dst = *src; > > > - return copy_packet_data(dst, src, 0); > > > + return av_packet_ref(dst, src); > > > } > > > > > > void av_packet_free_side_data(AVPacket *pkt) > > > diff --git a/libavformat/gif.c b/libavformat/gif.c > > > index e817121..4230756 100644 > > > --- a/libavformat/gif.c > > > +++ b/libavformat/gif.c > > > @@ -165,7 +165,7 @@ static int flush_packet(AVFormatContext *s, AVPacket > > > *new) > > > > > > avio_write(pb, pkt->data, pkt->size); > > > > > > - av_free_packet(gif->prev_pkt); > > > + av_packet_unref(gif->prev_pkt); > > > if (new) > > > av_copy_packet(gif->prev_pkt, new); > > > > > > @@ -191,7 +191,7 @@ static int gif_write_trailer(AVFormatContext *s) > > > AVIOContext *pb = s->pb; > > > > > > flush_packet(s, NULL); > > > - av_freep(&gif->prev_pkt); > > > + av_packet_unref(gif->prev_pkt); > > > avio_w8(pb, 0x3b); > > > > > > return 0; > > > > > > But it spits out a whole binch of: > > > > > > *** Error in `devel/libav/avconv': free(): invalid pointer: > > > 0x000000000a279bd0 *** > > > *** Error in `devel/libav/avconv': corrupted double-linked list: > > > 0x0000000009c715c0 *** > > > > > > and then it segfaults. The only difference I can see between > > > copy_packet_data() > > > and av_packet_ref() is the way side data is copied: the former uses > > > ffmpeg's > > > av_copy_packet_side_data() while the latter uses av_packet_copy_props(). > > > So I > > > replaced the av_packet_copy_props() call with the ffmpeg thing like this: > > > > > > > Aren't you confusing the AVPacket itself (which is a container for data) > > with > > the data it contains? > > Possibly. > > > The two must be allocated and freed separately, and av_packet_ref/unref only > > handles the data. > > Aren't they already? The prev_pkt is allocated with av_malloc() (though I > guess > I shouldn't have removed the av_freep()). > > > Check out the attached patch, it seems to work for me. > > Well, now it doesn't segfault or spit free() errors, but it still fails with: > > [gif @ 0x31b3820] Invalid palette extradata > av_interleaved_write_frame(): Invalid data found when processing input > > so it looks like the side data is still not copied properly. What fixes it is > the following: > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index c0a0f8c..f3edd37 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -327,20 +327,25 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket > *src) > dst->stream_index = src->stream_index; > dst->side_data_elems = src->side_data_elems; > > - for (i = 0; i < src->side_data_elems; i++) { > - enum AVPacketSideDataType type = src->side_data[i].type; > - int size = src->side_data[i].size; > - uint8_t *src_data = src->side_data[i].data; > - uint8_t *dst_data = av_packet_new_side_data(dst, type, size); > - > - if (!dst_data) { > - av_packet_free_side_data(dst); > - return AVERROR(ENOMEM); > + if (src->side_data_elems) { > + DUP_DATA(dst->side_data, src->side_data, > + src->side_data_elems * sizeof(*src->side_data), 0, > ALLOC_MALLOC); > + if (src != dst) { > + memset(dst->side_data, 0, > + src->side_data_elems * sizeof(*src->side_data)); > + } > + for (i = 0; i < src->side_data_elems; i++) { > + DUP_DATA(dst->side_data[i].data, src->side_data[i].data, > + src->side_data[i].size, 1, ALLOC_MALLOC); > + dst->side_data[i].size = src->side_data[i].size; > + dst->side_data[i].type = src->side_data[i].type; > } > - memcpy(dst_data, src_data, size); > } > - > + dst->side_data_elems = src->side_data_elems; > return 0; > + > +failed_alloc: > + return AVERROR(ENOMEM); > } > > i.e. doing what ffmpeg's av_copy_packet_side_data() does, but in > av_packet_copy_props(). Not sure if it's correct.
Just found a bug in av_packet_copy_props(), which almost surely causes this problem. Try the patch I just sent to the ML. -- Anton Khirnov _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
