On 3/11/2021 5:08 PM, Andreas Rheinhardt wrote:
James Almer:
On 3/11/2021 3:26 PM, Andreas Rheinhardt wrote:
James Almer:
On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
James Almer:
On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
James Almer:
On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
James Almer:
On 3/11/2021 1:11 PM, Marton Balint wrote:
On Thu, 11 Mar 2021, James Almer wrote:
This function acts as a replacement for both av_grow_packet()
and
av_shrink_packet(), the latter which is now deprecated and
will be
removed as
it does not correctly handle non-writable packets.
I don't think this is a good idea, av_shrink_packet cannot fail,
av_grow_packet can. By using the same function you are losing the
information if the end result should be checked or not.
I'm not sure i follow. av_shrink_packet() is not being changed at
all,
just deprecated, scheduled for removal, and its use discouraged.
I'd argue that a deprecation is actually a change.
Maybe i should have split this in two, one to add
av_packet_resize() and
one to deprecate av_shrink_packet(), to avoid confusion.
In any case, the fact av_shrink_packet() cannot fail is the
reason I'm
getting rid of it. It's zeroing the padding without bothering to
check
if the packet is writable at all. And we can't have it attempt to
make
it writable because it can't then report if it failed to
reallocate
the
buffer. So this patch here deprecates it for being a function that
predates reference counted buffers and is not able to properly
handle
them, and adds a replacement for it that also supersedes
av_grow_packet() while at it.
Yet you are not documenting that av_packet_resize can't fail if
it is
shrinking a packet known to be writable; ergo all unchecked uses of
this
function in the second and third patch are API abuse.
I can add checks for all of them if you prefer. I didn't because
they
are all internal uses known to (in theory) not fail.
No, I don't prefer checks for stuff that can't fail. I'd rather
prefer
if it were documented that it can't fail in these cases.
I'm in general against adding that kind of constrain on a function's
documentation because you never know how it or what it processes could
be extended in the future.
Right now it can't fail in that scenario, true, but in the future we
could add some feature to AVPacket that would need to be handled by
this
function that could start making it fail in that same scenario, and
suddenly, the doxy is no longer correct, and the function needs to be
replaced because it became unable to handle the new functionality.
If a function can fail at all, then the library user should always
make
sure to check the return value, and not be told there's one specific
scenario where they don't need to.
In this case I am against this patch.
To add to what explained said above, it's the entire reason making
av_shrink_packet() return void was a mistake. It worked great before
reference counted buffers, but it was short sighted and the function is
now technically unusable after they were introduced, and why we need to
replace it now.
This patchset wouldn't exist had it been designed to return an int.
The function is very well useable, but only for its use-case of already
writable packets. (Not documenting this when the refcounted API was
introduced was an error; or not deprecating it and replacing it with a
function that explicitly says so.)
It should have been deprecated and replaced back then, but wasn't. And
so I'm righting that wrong now.
What you're asking for is to make the exact same mistake again, and it
could very well come to bite us in the future, again. It's definitely
not a good reason to try and block this patch at all.
av_shrink_packet has four lines, two of which do the actual work. It is
extremely simple and could nearly be inlined. If any future extension
requires adding something that can fail to this, then said future
extension sounds like a huge step backwards.
Well, then you're calling reference counted buffers a step backwards,
because their addition required making a compliant shrink + zero padding
function be able to fail, as already explained.
Before the introduction of refcounted packets avformat.h contained:
"If AVPacket.destruct is set on the returned packet, then the packet is
allocated dynamically and the user may keep it indefinitely. Otherwise,
if AVPacket.destruct is NULL, the packet data is backed by a static
storage somewhere inside the demuxer and the packet is only valid until
the next av_read_frame() call or closing the file."
So there were non-ownership packets even before refcounting was a thing;
this means that av_shrink_packet was wrong even then. But for the
equivalent of writable packets (namely those with destruct set) it was
correct and this remained true even after the introduction of refcounted
packets.
Unnecessary constrains like the one you suggest are proven to be short
sighted and not future proof, and all just to let the caller not check a
return value in one very specific scenario.
This is not an unnecessary constraint. The owner of a writable packet is
allowed to write to it by definition.
Why do you think this is, or will always be, about "Writing"? This is
about "Shrinking" and "Resizing". Right now, using knowledge about
current internal workings and definitions of AVPacket, that means that
if writable, it can't fail. But tomorrow?
One needn't perform another check
to see whether one gets permission to write to it, one already has it
(as long as one respects the packet's size, which is automatically true
when shrinking a packet). So demanding that such an operation doesn't
fail is entirely reasonable and natural (as natural as expecting that
memset can't fail).
Tell that to whoever in five years comes up with new functionality that
can generate a failure in an AVPacket payload shrinking scenario
regardless of writability or ownership that will need to replace this
function because we arrogantly thought we knew better.
It's a constrain we gain nothing from by defining, while putting the
usability of this function in the long term at risk. It's literally one
very specific scenario you want users to be allowed to not check one
miserable return value, and you've spent an entire evening arguing about
it. I don't know about you but i think there are better things to spend
said time on.
As i mentioned before, there's precedent for this kind of assumption
ultimately making an API non future proof. There's no reason whatsoever
to spend so much time demanding we make the same mistake again. None.
Regards,
Marton
Signed-off-by: James Almer <jamr...@gmail.com>
---
libavcodec/avpacket.c | 19 +++++++++++++++----
libavcodec/packet.h | 16 ++++++++++++++++
libavcodec/version.h | 3 +++
3 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 32cb71fcf0..7d0dbadbed 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
return 0;
}
+#if FF_API_SHRINK_PACKET
void av_shrink_packet(AVPacket *pkt, int size)
{
if (pkt->size <= size)
@@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int
size)
pkt->size = size;
memset(pkt->data + size, 0,
AV_INPUT_BUFFER_PADDING_SIZE);
}
+#endif
int av_grow_packet(AVPacket *pkt, int grow_by)
{
- int new_size;
av_assert0((unsigned)pkt->size <= INT_MAX -
AV_INPUT_BUFFER_PADDING_SIZE);
if ((unsigned)grow_by >
INT_MAX - (pkt->size +
AV_INPUT_BUFFER_PADDING_SIZE))
return AVERROR(ENOMEM);
- new_size = pkt->size + grow_by +
AV_INPUT_BUFFER_PADDING_SIZE;
+ return av_packet_resize(pkt, pkt->size + grow_by);
+}
+
+int av_packet_resize(AVPacket *pkt, int size)
+{
+ int new_size;
+
+ if (size < 0 || size > INT_MAX -
AV_INPUT_BUFFER_PADDING_SIZE)
+ return AVERROR(EINVAL);
+
+ new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
if (pkt->buf) {
size_t data_offset;
uint8_t *old_data = pkt->data;
@@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int
grow_by)
if (!pkt->buf)
return AVERROR(ENOMEM);
if (pkt->size > 0)
- memcpy(pkt->buf->data, pkt->data, pkt->size);
+ memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
size));
pkt->data = pkt->buf->data;
}
- pkt->size += grow_by;
+ pkt->size = size;
memset(pkt->data + pkt->size, 0,
AV_INPUT_BUFFER_PADDING_SIZE);
return 0;
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index 3d9013d783..1720d973f5 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
*/
int av_new_packet(AVPacket *pkt, int size);
+#if FF_API_SHRINK_PACKET
/**
* Reduce packet size, correctly zeroing padding
*
* @param pkt packet
* @param size new size
+ *
+ * @deprecated Use av_packet_resize
*/
+attribute_deprecated
void av_shrink_packet(AVPacket *pkt, int size);
+#endif
+
+/**
+ * Resize the payload of a packet, correctly zeroing padding
and
avoiding data
+ * copy if possible.
+ *
+ * @param pkt packet
+ * @param size new size
+ *
+ * @return 0 on success, a negative AVERROR on error
+ */
+int av_packet_resize(AVPacket *pkt, int size);
/**
* Increase packet size, correctly zeroing padding
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 3124ec8061..6c362b43e2 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -162,5 +162,8 @@
#ifndef FF_API_GET_FRAME_CLASS
#define FF_API_GET_FRAME_CLASS (LIBAVCODEC_VERSION_MAJOR
< 60)
#endif
+#ifndef FF_API_SHRINK_PACKET
+#define FF_API_SHRINK_PACKET (LIBAVCODEC_VERSION_MAJOR <
60)
+#endif
#endif /* AVCODEC_VERSION_H */
--
2.30.2
_______________________________________________
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".
_______________________________________________
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".
_______________________________________________
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".
_______________________________________________
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".
