Marton Balint: > > > On Fri, 12 Mar 2021, James Almer wrote: > >>>> >>>> I get a packet from a demuxer. It contains two independent portions >>>> (NALUs, OBUs, etc) i want to separate in order to feed them to >>>> something like a multi threaded decoder. And so I create a new >>>> reference to it, resulting in two packets pointing to the same data. >>>> I shrink one to only contain the first portion, and i add the >>>> required byte offset to the data pointer and subtract it to the size >>>> field on the second packet so it contains only the second portion. >>>> The result if i use av_packet_resize() will be two valid, properly >>>> padded packets containing their respective expected data, but if i >>>> use av_shrink_packet(), the result will be the packet with the >>>> second portion featuring padding bytes worth of data zeroed at the >>>> start of its payload. >>> >>> This looks like an unfortunate example, since you are: >>> - adding a reference to the packet, so you don't have to duplicate data >>> - and then want to add zero padding to the partial packet, so you will >>> duplicate data. >>> And I think the padding does not have to be zero for the partial packet. >> >> The padding exists AFAIK because something, like an optimized >> bitstream reader that tries to process several bytes at the same time, >> may end up reading or writing pass the reported end of the buffer. >> That means that if reading and it's not all zeroes, it could in theory >> have unpredictable results. Hence why everything always zeroes the >> padding, even av_shrink_packet(). >> >> And yes, what you describe is what some bitstream filters and the >> matroska demuxer do. They just create several packet references >> pointing to the same data buffer, but using different offsets for the >> data pointer. They all have "padding", but in many cases said padding >> is the beginning of the payload of another packet, and that's not ideal. > > Well, performance reasons come in play and the ability to not copy the > data. Yeah, it does not play nicely with our historical requirement of > zero padding. > > I did a little experimenting, and except for subtitles (which have > crashed and burned because of the missing 0 string terminator), most > tests handled non-zero padding well, a few failed tests, mostly for > partial source files, for which I guess it is inevitable? > > So I guess for now we will stay in the gray area of "if it does not > crash, then having non-zero padding for some partial packets is OKish"... > >>> I agree that it is not nice that av_shrink_packet() writes something >>> to the packet, people may not think about it and misuse it instead of >>> directly decreaseing pkt->size when they need a partial packet. That >>> is why I suggested the assert(). One might argue that it kind of >>> breaks behaviour, but I'd say it does not break it too much, and >>> writing to a non-writable packet was broken in the first place. >>> >>> Alternatively we can change av_shrink_packet() to only zero the >>> padding if the packet is writable, which has the benefit that it will >>> do what people generally expect, no matter if you throw a writable or >>> a non-writable packet to it. >>> >>> A third alternative is to introduce void av_shrink_packet2() in which >>> you explicitly document that a writable packet is needed and do the >>> assert there, and deprecate av_shrink_packet(). >> >> Not a fan of functions with a 2 suffix. And to be honest, I really >> don't care about what we do with av_shrink_packet(). >> Do you want to keep it around? Ok. Want to deprecate and remove it? >> Better. Want to add an assert to it? Not a fan and it may result in >> unexpected crashes for library users, but whatever. >> >> I just want to add av_packet_resize() to have a single resize function >> that will properly handle AVPackets in their current and any potential >> future states. > > Ok, then I suggest you add av_resize_packet but keep av_shrink_packet() > as well, and we can add an assert() to it after the release/bump. >
Can we really add an assert to it? I am not so sure about that. The problem lies in ff_alloc_packet2(): It can return non-refcounted packets whose data points to avctx->internal->byte_buffer. (Some encoders need to allocate big buffers for worst-case scenarios and then the data is initially written to said byte_buffer and copied lateron via av_packet_make_refcounted() in encode_simple_internal().) This is basically what Anton said: Not all parts of the code completely abide by the constraints of the AVBuffer API; and they can still work fine if they abide by their own notions of ownership. - Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
