Hi, On Wed, Apr 13, 2011 at 11:37 AM, Kostya <[email protected]> wrote: > On Wed, Apr 13, 2011 at 11:19:26AM -0400, Ronald S. Bultje wrote: >> Hi, >> >> On Wed, Apr 13, 2011 at 11:14 AM, Kostya <[email protected]> wrote: >> > On Wed, Apr 13, 2011 at 11:03:26AM -0400, Ronald S. Bultje wrote: >> >> On Wed, Apr 13, 2011 at 10:33 AM, Kostya <[email protected]> >> >> wrote: >> >> > Here are improved patches. >> >> [..] >> >> > + * Additional packet data that can be provided by the container. >> >> > + * Packet can contain several types of side information. >> >> > + */ >> >> > + struct { >> >> > + uint8_t *data; >> >> > + int size; >> >> > + int type; >> >> >> >> enum AVSideDataType. Also the palette-type is missing here (you >> >> probably put that in the second patch. I'd put it here just so it's >> >> clear from the patch what this is to be used for). >> > >> > moved >> > >> >> [..] >> >> > +#define DUP_DATA(dst, size, tmp, padding) \ >> >> [..] >> >> > + tmp = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); \ >> >> > + } else { \ >> >> > + tmp = av_malloc(size); \ >> >> > + } \ >> >> > + if (!data) \ >> >> > + return AVERROR(ENOMEM); \ >> >> >> >> s/data/tmp/? >> >> >> >> Also the declaration of tmp outside the macro is kinda ugly, why not >> >> just declare it inside the macro as void *tmp? >> > >> > done >> [..] >> > +#define DUP_DATA(dst, size, padding) \ >> > + do { \ >> > + void *data; \ >> > + if (padding) { \ >> > + if ((unsigned)(size) > (unsigned)(size) + >> > FF_INPUT_BUFFER_PADDING_SIZE) \ >> > + return AVERROR(ENOMEM); \ >> > + data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); \ >> > + } else { \ >> > + data = av_malloc(size); \ >> > + } \ >> > + if (!data) \ >> > + return AVERROR(ENOMEM); \ >> > + memcpy(data, dst, size); \ >> > + if (padding) \ >> > + memset((uint8_t*)data + size, 0, >> > FF_INPUT_BUFFER_PADDING_SIZE); \ >> > + dst = data; \ >> > + } while(0) >> > + >> > int av_dup_packet(AVPacket *pkt) >> > { >> > if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct >> > == NULL)) && pkt->data) { >> > - uint8_t *data; >> > - /* We duplicate the packet and don't forget to add the padding >> > again. */ >> > - if((unsigned)pkt->size > (unsigned)pkt->size + >> > FF_INPUT_BUFFER_PADDING_SIZE) >> > - return AVERROR(ENOMEM); >> > - data = av_malloc(pkt->size + FF_INPUT_BUFFER_PADDING_SIZE); >> > - if (!data) { >> > - return AVERROR(ENOMEM); >> > - } >> > - memcpy(data, pkt->data, pkt->size); >> > - memset(data + pkt->size, 0, FF_INPUT_BUFFER_PADDING_SIZE); >> > - pkt->data = data; >> > + DUP_DATA(pkt->data, pkt->size, 1); >> > pkt->destruct = av_destruct_packet; >> > + >> > + if (pkt->side_data_elems) { >> > + int i; >> > + >> > + DUP_DATA(pkt->side_data, pkt->side_data_elems * >> > sizeof(*pkt->side_data), 0); >> > + for (i = 0; i < pkt->side_data_elems; i++) { >> > + DUP_DATA(pkt->side_data[i].data, pkt->side_data[i].size, >> > 1); >> > + } >> > + } >> > } >> >> So if any of these malloc()s fails, who is responsible for cleaning up >> the mess? I assume that av_packet_free() is supposed to do the right >> thing, right? > > don't know > >> Currently, if the first malloc() fails, then the side-data is still >> pointing to the original packet's side-data and will free it, thus >> doing horrible stuff. Same, if the side-data or any of the chunks >> inside it malloc() fails, the side-data still points to the old >> side-data or chunks and will free() it, thus leading to horrible >> outcomes. > > I thought it's no worse than current scheme - what happens to duplicate data > pointer if it's not reallocated. Will it be double-freed or just ignored? > Anyway, here's the cautious av_dup_packet(), feel free to squash into first > patch.
This looks OK to me. Haven't looked at 2nd patch yet. Ronald _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
