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