| Message ID | MZ9v9ux--3-2@lynne.ee |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avpacket: ABI bump additions | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure | warning | Failed to apply patch |
On Sun, 25 Apr 2021, Lynne wrote: > This is the same patch sent in January, rebased on top of the ABI bump > patchset. > > Two additions mirror exactly what AVFrame has - an opaque field > and an opaque_ref for user-side private data. > For justification on the void *opaque field, you can read the archives, > since the question was brought up in January. > > As for the time_base field, for now, it will only be used to inform the user, > and will not alter the behavior of the libraries. That change will come as an > optional flag. I would like to see some documentation and code which shows when the time_base is respected and when it is not before it is added to AVPacket. After the bump we usually have a 1-2 months of cooldown when the ABI can change, so there really is no rush. Thanks, Marton
Lynne: > From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 > From: Lynne <dev@lynne.ee> > Date: Sat, 23 Jan 2021 19:56:18 +0100 > Subject: [PATCH] avpacket: ABI bump additions > > --- > libavcodec/avpacket.c | 5 +++++ > libavcodec/packet.h | 21 +++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index e32c467586..03b73b3b53 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) > dst->flags = src->flags; > dst->stream_index = src->stream_index; > > + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); > + if (i < 0) > + return i; 1. Don't use i here; add a new variable. 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the destination packet as uninitialized and make no attempt at unreferencing its content; yet you try to reuse opaque_ref. Even worse, you might return potentially dangerous packets: If the properties were uninitialized and av_packet_copy_props() failed, then the caller were not allowed to unreference the packet even when the non-properties were set to sane values. The easiest way to fix this is to move setting opaque ref to the place after initializing side_data below and either set dst->opaque_ref to NULL before av_buffer_replace() or to not use av_buffer_replace(). It may also be best to unref it again if copying side data fails. > + > dst->side_data = NULL; > dst->side_data_elems = 0; > for (i = 0; i < src->side_data_elems; i++) { > @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) > void av_packet_unref(AVPacket *pkt) > { > av_packet_free_side_data(pkt); > + av_buffer_unref(&pkt->opaque_ref); > av_buffer_unref(&pkt->buf); > get_packet_defaults(pkt); > } > diff --git a/libavcodec/packet.h b/libavcodec/packet.h > index fad8341c12..c29ad18a2b 100644 > --- a/libavcodec/packet.h > +++ b/libavcodec/packet.h > @@ -383,6 +383,27 @@ typedef struct AVPacket { > int64_t duration; > > int64_t pos; ///< byte position in stream, -1 if unknown > + > + /** > + * for some private data of the user > + */ > + void *opaque; The corresponding AVFrame field is copied when copying props. > + > + /** > + * AVBufferRef for free use by the API user. FFmpeg will never check the > + * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when > + * the packet is unreferenced. av_packet_copy_props() calls create a new > + * reference with av_buffer_ref() for the target packet's opaque_ref field. > + * > + * This is unrelated to the opaque field, although it serves a similar > + * purpose. > + */ > + AVBufferRef *opaque_ref; > + > + /** > + * Time base of the packet's timestamps. > + */ > + AVRational time_base; > } AVPacket; > > #if FF_API_INIT_PACKET
Apr 26, 2021, 02:36 by cus@passwd.hu: > > > On Sun, 25 Apr 2021, Lynne wrote: > >> This is the same patch sent in January, rebased on top of the ABI bump >> patchset. >> >> Two additions mirror exactly what AVFrame has - an opaque field >> and an opaque_ref for user-side private data. >> For justification on the void *opaque field, you can read the archives, >> since the question was brought up in January. >> >> As for the time_base field, for now, it will only be used to inform the user, >> and will not alter the behavior of the libraries. That change will come as an >> optional flag. >> > > I would like to see some documentation and code which shows when the time_base is respected and when it is not before it is added to AVPacket. After the bump we usually have a 1-2 months of cooldown when the ABI can change, so there really is no rush. > I didn't mean to say that it'll be done during the bump, but rather after it. Once we add time_base, there are no ABI-breaking changes to support what I was thinking of. Or do you still want to see some code? It's not a small amount of work.
On 4/25/2021 10:27 PM, Andreas Rheinhardt wrote: > Lynne: >> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >> From: Lynne <dev@lynne.ee> >> Date: Sat, 23 Jan 2021 19:56:18 +0100 >> Subject: [PATCH] avpacket: ABI bump additions >> >> --- >> libavcodec/avpacket.c | 5 +++++ >> libavcodec/packet.h | 21 +++++++++++++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >> index e32c467586..03b73b3b53 100644 >> --- a/libavcodec/avpacket.c >> +++ b/libavcodec/avpacket.c >> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >> dst->flags = src->flags; >> dst->stream_index = src->stream_index; >> >> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >> + if (i < 0) >> + return i; > > 1. Don't use i here; add a new variable. > 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the > destination packet as uninitialized and make no attempt at unreferencing > its content; yet you try to reuse opaque_ref. Even worse, you might > return potentially dangerous packets: If the properties were > uninitialized and av_packet_copy_props() failed, then the caller were > not allowed to unreference the packet even when the non-properties were > set to sane values. The easiest way to fix this is to move setting > opaque ref to the place after initializing side_data below and either > set dst->opaque_ref to NULL before av_buffer_replace() or to not use > av_buffer_replace(). It may also be best to unref it again if copying > side data fails. Fwiw, the idea is that once sizeof(AVPacket) is not a part of the ABI anymore, there will no longer be such thing as an uninitialized packet as far as proper API usage goes. And as such the lines below... > >> + >> dst->side_data = NULL; >> dst->side_data_elems = 0; ^^^^^^^^^^^^^^^^^^^^ ...should be replaced by a av_packet_free_side_data(dst) call. Similarly, at that point using av_buffer_replace() on opaque_ref should become safe. But until then, you're right that doing so *will* go wrong on uninitialized packets. >> for (i = 0; i < src->side_data_elems; i++) { >> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >> void av_packet_unref(AVPacket *pkt) >> { >> av_packet_free_side_data(pkt); >> + av_buffer_unref(&pkt->opaque_ref); >> av_buffer_unref(&pkt->buf); >> get_packet_defaults(pkt); >> } >> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >> index fad8341c12..c29ad18a2b 100644 >> --- a/libavcodec/packet.h >> +++ b/libavcodec/packet.h >> @@ -383,6 +383,27 @@ typedef struct AVPacket { >> int64_t duration; >> >> int64_t pos; ///< byte position in stream, -1 if unknown >> + >> + /** >> + * for some private data of the user >> + */ >> + void *opaque; > > The corresponding AVFrame field is copied when copying props. > >> + >> + /** >> + * AVBufferRef for free use by the API user. FFmpeg will never check the >> + * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when >> + * the packet is unreferenced. av_packet_copy_props() calls create a new >> + * reference with av_buffer_ref() for the target packet's opaque_ref field. >> + * >> + * This is unrelated to the opaque field, although it serves a similar >> + * purpose. >> + */ >> + AVBufferRef *opaque_ref; >> + >> + /** >> + * Time base of the packet's timestamps. >> + */ >> + AVRational time_base; >> } AVPacket; >> >> #if FF_API_INIT_PACKET > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Mon, 26 Apr 2021, Lynne wrote: > Apr 26, 2021, 02:36 by cus@passwd.hu: > >> >> >> On Sun, 25 Apr 2021, Lynne wrote: >> >>> This is the same patch sent in January, rebased on top of the ABI bump >>> patchset. >>> >>> Two additions mirror exactly what AVFrame has - an opaque field >>> and an opaque_ref for user-side private data. >>> For justification on the void *opaque field, you can read the archives, >>> since the question was brought up in January. >>> >>> As for the time_base field, for now, it will only be used to inform the user, >>> and will not alter the behavior of the libraries. That change will come as an >>> optional flag. >>> >> >> I would like to see some documentation and code which shows when the time_base is respected and when it is not before it is added to AVPacket. After the bump we usually have a 1-2 months of cooldown when the ABI can change, so there really is no rush. >> > > I didn't mean to say that it'll be done during the bump, but rather after it. > Once we add time_base, there are no ABI-breaking changes to support > what I was thinking of. > Or do you still want to see some code? It's not a small amount of work. But if it is far from trivial, then does it really worth introducing it? I am asking, because I am still not sure about the benefits or the issues it is trying to solve. You wrote earlier that you don't like that you have to pass packets to the muxer in a timebase as set by the muxer's init function. Solving this by adding a muxer open flag which saves the preferred time base of the user and rescales all packets from the user's preferred time base to the real time base before processing seems much more managable than introducing the AVPacket->time_base support everywhere and as far as I see it solves this problem just the same. Are there similar problems elsewhere? If there are, then is it not more managable to allow the user to specify a preferred input or output timebase during init instead of allowing per-packet timebases? By adding time_base to AVPacket you basically say that it is possible that each packet of a stream have a different time base. But in realitly, this never happens. Regards, Marton
Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: > Lynne: > >> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >> From: Lynne <dev@lynne.ee> >> Date: Sat, 23 Jan 2021 19:56:18 +0100 >> Subject: [PATCH] avpacket: ABI bump additions >> >> --- >> libavcodec/avpacket.c | 5 +++++ >> libavcodec/packet.h | 21 +++++++++++++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >> index e32c467586..03b73b3b53 100644 >> --- a/libavcodec/avpacket.c >> +++ b/libavcodec/avpacket.c >> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >> dst->flags = src->flags; >> dst->stream_index = src->stream_index; >> >> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >> + if (i < 0) >> + return i; >> > > 1. Don't use i here; add a new variable. > 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the > destination packet as uninitialized and make no attempt at unreferencing > its content; yet you try to reuse opaque_ref. Even worse, you might > return potentially dangerous packets: If the properties were > uninitialized and av_packet_copy_props() failed, then the caller were > not allowed to unreference the packet even when the non-properties were > set to sane values. The easiest way to fix this is to move setting > opaque ref to the place after initializing side_data below and either > set dst->opaque_ref to NULL before av_buffer_replace() or to not use > av_buffer_replace(). It may also be best to unref it again if copying > side data fails. > >> + >> dst->side_data = NULL; >> dst->side_data_elems = 0; >> for (i = 0; i < src->side_data_elems; i++) { >> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >> void av_packet_unref(AVPacket *pkt) >> { >> av_packet_free_side_data(pkt); >> + av_buffer_unref(&pkt->opaque_ref); >> av_buffer_unref(&pkt->buf); >> get_packet_defaults(pkt); >> } >> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >> index fad8341c12..c29ad18a2b 100644 >> --- a/libavcodec/packet.h >> +++ b/libavcodec/packet.h >> @@ -383,6 +383,27 @@ typedef struct AVPacket { >> int64_t duration; >> >> int64_t pos; ///< byte position in stream, -1 if unknown >> + >> + /** >> + * for some private data of the user >> + */ >> + void *opaque; >> > > The corresponding AVFrame field is copied when copying props. > Fixed both, thanks. Also copied the time_base field and made av_init_packet() initialize all fields. Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes.
3 Jun 2021, 06:58 by dev@lynne.ee: > Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: > >> Lynne: >> >>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>> From: Lynne <dev@lynne.ee> >>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>> Subject: [PATCH] avpacket: ABI bump additions >>> >>> --- >>> libavcodec/avpacket.c | 5 +++++ >>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>> index e32c467586..03b73b3b53 100644 >>> --- a/libavcodec/avpacket.c >>> +++ b/libavcodec/avpacket.c >>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>> dst->flags = src->flags; >>> dst->stream_index = src->stream_index; >>> >>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>> + if (i < 0) >>> + return i; >>> >> >> 1. Don't use i here; add a new variable. >> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >> destination packet as uninitialized and make no attempt at unreferencing >> its content; yet you try to reuse opaque_ref. Even worse, you might >> return potentially dangerous packets: If the properties were >> uninitialized and av_packet_copy_props() failed, then the caller were >> not allowed to unreference the packet even when the non-properties were >> set to sane values. The easiest way to fix this is to move setting >> opaque ref to the place after initializing side_data below and either >> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >> av_buffer_replace(). It may also be best to unref it again if copying >> side data fails. >> >>> + >>> dst->side_data = NULL; >>> dst->side_data_elems = 0; >>> for (i = 0; i < src->side_data_elems; i++) { >>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>> void av_packet_unref(AVPacket *pkt) >>> { >>> av_packet_free_side_data(pkt); >>> + av_buffer_unref(&pkt->opaque_ref); >>> av_buffer_unref(&pkt->buf); >>> get_packet_defaults(pkt); >>> } >>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>> index fad8341c12..c29ad18a2b 100644 >>> --- a/libavcodec/packet.h >>> +++ b/libavcodec/packet.h >>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>> int64_t duration; >>> >>> int64_t pos; ///< byte position in stream, -1 if unknown >>> + >>> + /** >>> + * for some private data of the user >>> + */ >>> + void *opaque; >>> >> >> The corresponding AVFrame field is copied when copying props. >> > > Fixed both, thanks. Also copied the time_base field and made av_init_packet() > initialize all fields. > Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes. > Since no one yet has objected, will push this in 3 days unless someone does.
On Tue, 6 Jul 2021, Lynne wrote: > 3 Jun 2021, 06:58 by dev@lynne.ee: > >> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >> >>> Lynne: >>> >>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>> From: Lynne <dev@lynne.ee> >>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>> Subject: [PATCH] avpacket: ABI bump additions >>>> >>>> --- >>>> libavcodec/avpacket.c | 5 +++++ >>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>> 2 files changed, 26 insertions(+) >>>> >>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>> index e32c467586..03b73b3b53 100644 >>>> --- a/libavcodec/avpacket.c >>>> +++ b/libavcodec/avpacket.c >>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>> dst->flags = src->flags; >>>> dst->stream_index = src->stream_index; >>>> >>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>> + if (i < 0) >>>> + return i; >>>> >>> >>> 1. Don't use i here; add a new variable. >>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>> destination packet as uninitialized and make no attempt at unreferencing >>> its content; yet you try to reuse opaque_ref. Even worse, you might >>> return potentially dangerous packets: If the properties were >>> uninitialized and av_packet_copy_props() failed, then the caller were >>> not allowed to unreference the packet even when the non-properties were >>> set to sane values. The easiest way to fix this is to move setting >>> opaque ref to the place after initializing side_data below and either >>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>> av_buffer_replace(). It may also be best to unref it again if copying >>> side data fails. >>> >>>> + >>>> dst->side_data = NULL; >>>> dst->side_data_elems = 0; >>>> for (i = 0; i < src->side_data_elems; i++) { >>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>> void av_packet_unref(AVPacket *pkt) >>>> { >>>> av_packet_free_side_data(pkt); >>>> + av_buffer_unref(&pkt->opaque_ref); >>>> av_buffer_unref(&pkt->buf); >>>> get_packet_defaults(pkt); >>>> } >>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>> index fad8341c12..c29ad18a2b 100644 >>>> --- a/libavcodec/packet.h >>>> +++ b/libavcodec/packet.h >>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>> int64_t duration; >>>> >>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>> + >>>> + /** >>>> + * for some private data of the user >>>> + */ >>>> + void *opaque; >>>> >>> >>> The corresponding AVFrame field is copied when copying props. >>> >> >> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >> initialize all fields. >> Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes. >> > > Since no one yet has objected, will push this in 3 days unless someone does. Can you reply this this please? http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html Thanks, Marton
6 Jul 2021, 21:57 by cus@passwd.hu: > > > On Tue, 6 Jul 2021, Lynne wrote: > >> 3 Jun 2021, 06:58 by dev@lynne.ee: >> >>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>> >>>> Lynne: >>>> >>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>>> From: Lynne <dev@lynne.ee> >>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>> >>>>> --- >>>>> libavcodec/avpacket.c | 5 +++++ >>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>> 2 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>> index e32c467586..03b73b3b53 100644 >>>>> --- a/libavcodec/avpacket.c >>>>> +++ b/libavcodec/avpacket.c >>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>> dst->flags = src->flags; >>>>> dst->stream_index = src->stream_index; >>>>> >>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>> + if (i < 0) >>>>> + return i; >>>>> >>>> >>>> 1. Don't use i here; add a new variable. >>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>> destination packet as uninitialized and make no attempt at unreferencing >>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>> return potentially dangerous packets: If the properties were >>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>> not allowed to unreference the packet even when the non-properties were >>>> set to sane values. The easiest way to fix this is to move setting >>>> opaque ref to the place after initializing side_data below and either >>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>> av_buffer_replace(). It may also be best to unref it again if copying >>>> side data fails. >>>> >>>>> + >>>>> dst->side_data = NULL; >>>>> dst->side_data_elems = 0; >>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>> void av_packet_unref(AVPacket *pkt) >>>>> { >>>>> av_packet_free_side_data(pkt); >>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>> av_buffer_unref(&pkt->buf); >>>>> get_packet_defaults(pkt); >>>>> } >>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>> index fad8341c12..c29ad18a2b 100644 >>>>> --- a/libavcodec/packet.h >>>>> +++ b/libavcodec/packet.h >>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>> int64_t duration; >>>>> >>>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>>> + >>>>> + /** >>>>> + * for some private data of the user >>>>> + */ >>>>> + void *opaque; >>>>> >>>> >>>> The corresponding AVFrame field is copied when copying props. >>>> >>> >>> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >>> initialize all fields. >>> Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes. >>> >> >> Since no one yet has objected, will push this in 3 days unless someone does. >> > > Can you reply this this please? > > http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html > > You wrote earlier that you don't like that you have to pass packets to the > muxer in a timebase as set by the muxer's init function. Solving this by > adding a muxer open flag which saves the preferred time base of the user > and rescales all packets from the user's preferred time base to the real > time base before processing seems much more managable than introducing the > AVPacket->time_base support everywhere and as far as I see it solves this > problem just the same. I'm mainly introducing the field for myself. I have some (de)muxer code that's timestamp dependent, and timestamps don't make sense without knowing the time base. Since multiple streams go into that code, having a sideband way to plumb the timebases made the code very messy. The fact that they can also be used to save on an awkward and complicated mechanism to negotiate just comes as a bonus. Honestly, I had to read the mpv source code to realize that this is the correct sequence that has to happen, when none of this is even necessary. I'm sure other API users will find the field useful. I don't mind having a way to set the preferred time base, but I do think it'll be even more confusing if it converts time bases as well. We need a way to give a hint to muxers what time base you'd like, but I'd rather have it just remain a hint rather than also do the conversion, since it'll limit the usability to just your stream's input timebase. And if you have to specify your stream input timebase somewhere, then this would be better, where it's relevant. > Are there similar problems elsewhere? If there are, then is it not more > managable to allow the user to specify a preferred input or output > timebase during init instead of allowing per-packet timebases? By adding > time_base to AVPacket you basically say that it is possible > that each packet of a stream have a different time base. But in realitly, > this never happens. We can add a comment saying this will never happen. Although if we do decide to allow packet timebase to dynamically change, and we add a similar field to frames (which I plan to do after this, but since we can add fields to AVFrames easily, I left it for later), then we would be able to dynamically handle more without effort from the user. For example, streams could be switched and concatenated in a way that doesn't break demuxing. elenril has some plans on writing a concat bsf, so he can say whether that could be useful there.
On Thu, Jun 03, 2021 at 06:58:56AM +0200, Lynne wrote: > Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: > > > Lynne: > > > >> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 > >> From: Lynne <dev@lynne.ee> > >> Date: Sat, 23 Jan 2021 19:56:18 +0100 > >> Subject: [PATCH] avpacket: ABI bump additions > >> > >> --- > >> libavcodec/avpacket.c | 5 +++++ > >> libavcodec/packet.h | 21 +++++++++++++++++++++ > >> 2 files changed, 26 insertions(+) > >> > >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > >> index e32c467586..03b73b3b53 100644 > >> --- a/libavcodec/avpacket.c > >> +++ b/libavcodec/avpacket.c > >> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) > >> dst->flags = src->flags; > >> dst->stream_index = src->stream_index; > >> > >> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); > >> + if (i < 0) > >> + return i; > >> > > > > 1. Don't use i here; add a new variable. > > 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the > > destination packet as uninitialized and make no attempt at unreferencing > > its content; yet you try to reuse opaque_ref. Even worse, you might > > return potentially dangerous packets: If the properties were > > uninitialized and av_packet_copy_props() failed, then the caller were > > not allowed to unreference the packet even when the non-properties were > > set to sane values. The easiest way to fix this is to move setting > > opaque ref to the place after initializing side_data below and either > > set dst->opaque_ref to NULL before av_buffer_replace() or to not use > > av_buffer_replace(). It may also be best to unref it again if copying > > side data fails. > > > >> + > >> dst->side_data = NULL; > >> dst->side_data_elems = 0; > >> for (i = 0; i < src->side_data_elems; i++) { > >> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) > >> void av_packet_unref(AVPacket *pkt) > >> { > >> av_packet_free_side_data(pkt); > >> + av_buffer_unref(&pkt->opaque_ref); > >> av_buffer_unref(&pkt->buf); > >> get_packet_defaults(pkt); > >> } > >> diff --git a/libavcodec/packet.h b/libavcodec/packet.h > >> index fad8341c12..c29ad18a2b 100644 > >> --- a/libavcodec/packet.h > >> +++ b/libavcodec/packet.h > >> @@ -383,6 +383,27 @@ typedef struct AVPacket { > >> int64_t duration; > >> > >> int64_t pos; ///< byte position in stream, -1 if unknown > >> + > >> + /** > >> + * for some private data of the user > >> + */ > >> + void *opaque; > >> > > > > The corresponding AVFrame field is copied when copying props. > > > > Fixed both, thanks. Also copied the time_base field and made av_init_packet() > initialize all fields. > Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes. > > avpacket.c | 14 +++++++++++++- > packet.h | 21 +++++++++++++++++++++ > 2 files changed, 34 insertions(+), 1 deletion(-) > eb01b7d5d36d3bad80b01af192e0d2cb1060ab48 0001-avpacket-ABI-bump-additions.patch > From 9ba82d84a6686069f20a6c700c3605af8d894976 Mon Sep 17 00:00:00 2001 > From: Lynne <dev@lynne.ee> > Date: Sat, 23 Jan 2021 19:56:18 +0100 > Subject: [PATCH] avpacket: ABI bump additions > > --- Can you make the commit message more verbose ? if i saw this i would have no clue what it is about [...] > + /** > + * Time base of the packet's timestamps. > + */ > + AVRational time_base; IIUC the usecase for this is so users do not need to keep track of AVStreams to interpret timestamps ? Assuming iam correct, is this actually enough ? 2 streams could have different points in time for their 0 point too. thx [...]
On Wed, 7 Jul 2021, Lynne wrote: > 6 Jul 2021, 21:57 by cus@passwd.hu: > >> >> >> On Tue, 6 Jul 2021, Lynne wrote: >> >>> 3 Jun 2021, 06:58 by dev@lynne.ee: >>> >>>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>>> >>>>> Lynne: >>>>> >>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>>>> From: Lynne <dev@lynne.ee> >>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>>> >>>>>> --- >>>>>> libavcodec/avpacket.c | 5 +++++ >>>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>>> 2 files changed, 26 insertions(+) >>>>>> >>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>> index e32c467586..03b73b3b53 100644 >>>>>> --- a/libavcodec/avpacket.c >>>>>> +++ b/libavcodec/avpacket.c >>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>>> dst->flags = src->flags; >>>>>> dst->stream_index = src->stream_index; >>>>>> >>>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>>> + if (i < 0) >>>>>> + return i; >>>>>> >>>>> >>>>> 1. Don't use i here; add a new variable. >>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>>> destination packet as uninitialized and make no attempt at unreferencing >>>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>>> return potentially dangerous packets: If the properties were >>>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>>> not allowed to unreference the packet even when the non-properties were >>>>> set to sane values. The easiest way to fix this is to move setting >>>>> opaque ref to the place after initializing side_data below and either >>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>>> av_buffer_replace(). It may also be best to unref it again if copying >>>>> side data fails. >>>>> >>>>>> + >>>>>> dst->side_data = NULL; >>>>>> dst->side_data_elems = 0; >>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>>> void av_packet_unref(AVPacket *pkt) >>>>>> { >>>>>> av_packet_free_side_data(pkt); >>>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>>> av_buffer_unref(&pkt->buf); >>>>>> get_packet_defaults(pkt); >>>>>> } >>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>>> index fad8341c12..c29ad18a2b 100644 >>>>>> --- a/libavcodec/packet.h >>>>>> +++ b/libavcodec/packet.h >>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>>> int64_t duration; >>>>>> >>>>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>>>> + >>>>>> + /** >>>>>> + * for some private data of the user >>>>>> + */ >>>>>> + void *opaque; >>>>>> >>>>> >>>>> The corresponding AVFrame field is copied when copying props. >>>>> >>>> >>>> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >>>> initialize all fields. >>>> Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes. >>>> >>> >>> Since no one yet has objected, will push this in 3 days unless someone does. >>> >> >> Can you reply this this please? >> >> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html >> > >> You wrote earlier that you don't like that you have to pass packets to the >> muxer in a timebase as set by the muxer's init function. Solving this by >> adding a muxer open flag which saves the preferred time base of the user >> and rescales all packets from the user's preferred time base to the real >> time base before processing seems much more managable than introducing the >> AVPacket->time_base support everywhere and as far as I see it solves this >> problem just the same. > > I'm mainly introducing the field for myself. I have some (de)muxer code that's > timestamp dependent, and timestamps don't make sense without knowing the > time base. Since multiple streams go into that code, having a sideband way > to plumb the timebases made the code very messy. The fact that they can > also be used to save on an awkward and complicated mechanism to > negotiate just comes as a bonus. Honestly, I had to read the mpv source > code to realize that this is the correct sequence that has to happen, > when none of this is even necessary. I'm sure other API users will find the > field useful. > > I don't mind having a way to set the preferred time base, but I do think it'll > be even more confusing if it converts time bases as well. We need a way > to give a hint to muxers what time base you'd like, but I'd rather have it > just remain a hint rather than also do the conversion, since it'll limit the > usability to just your stream's input timebase. And if you have to specify your > stream input timebase somewhere, then this would be better, where it's > relevant. > >> Are there similar problems elsewhere? If there are, then is it not more >> managable to allow the user to specify a preferred input or output >> timebase during init instead of allowing per-packet timebases? By adding >> time_base to AVPacket you basically say that it is possible >> that each packet of a stream have a different time base. But in realitly, >> this never happens. > > We can add a comment saying this will never happen. Although if > we do decide to allow packet timebase to dynamically change, > and we add a similar field to frames (which I plan to do after this, but since > we can add fields to AVFrames easily, I left it for later), then we would be > able to dynamically handle more without effort from the user. > For example, streams could be switched and concatenated in a way > that doesn't break demuxing. elenril has some plans on writing a concat bsf, > so he can say whether that could be useful there. OK, thanks. I can't say I am fully convinced, but apparently nobody shares my concenrs, so feel free to go ahead with it. Regards, Marton
On 7/7/2021 3:12 PM, Marton Balint wrote: > > > On Wed, 7 Jul 2021, Lynne wrote: > >> 6 Jul 2021, 21:57 by cus@passwd.hu: >> >>> >>> >>> On Tue, 6 Jul 2021, Lynne wrote: >>> >>>> 3 Jun 2021, 06:58 by dev@lynne.ee: >>>> >>>>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>>>> >>>>>> Lynne: >>>>>> >>>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 >>>>>>> 2001 >>>>>>> From: Lynne <dev@lynne.ee> >>>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>>>> >>>>>>> --- >>>>>>> libavcodec/avpacket.c | 5 +++++ >>>>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>>>> 2 files changed, 26 insertions(+) >>>>>>> >>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>>> index e32c467586..03b73b3b53 100644 >>>>>>> --- a/libavcodec/avpacket.c >>>>>>> +++ b/libavcodec/avpacket.c >>>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, >>>>>>> const AVPacket *src) >>>>>>> dst->flags = src->flags; >>>>>>> dst->stream_index = src->stream_index; >>>>>>> >>>>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>>>> + if (i < 0) >>>>>>> + return i; >>>>>>> >>>>>> >>>>>> 1. Don't use i here; add a new variable. >>>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>>>> destination packet as uninitialized and make no attempt at >>>>>> unreferencing >>>>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>>>> return potentially dangerous packets: If the properties were >>>>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>>>> not allowed to unreference the packet even when the non-properties >>>>>> were >>>>>> set to sane values. The easiest way to fix this is to move setting >>>>>> opaque ref to the place after initializing side_data below and either >>>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>>>> av_buffer_replace(). It may also be best to unref it again if copying >>>>>> side data fails. >>>>>> >>>>>>> + >>>>>>> dst->side_data = NULL; >>>>>>> dst->side_data_elems = 0; >>>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const >>>>>>> AVPacket *src) >>>>>>> void av_packet_unref(AVPacket *pkt) >>>>>>> { >>>>>>> av_packet_free_side_data(pkt); >>>>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>>>> av_buffer_unref(&pkt->buf); >>>>>>> get_packet_defaults(pkt); >>>>>>> } >>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>>>> index fad8341c12..c29ad18a2b 100644 >>>>>>> --- a/libavcodec/packet.h >>>>>>> +++ b/libavcodec/packet.h >>>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>>>> int64_t duration; >>>>>>> >>>>>>> int64_t pos; ///< byte position in >>>>>>> stream, -1 if unknown >>>>>>> + >>>>>>> + /** >>>>>>> + * for some private data of the user >>>>>>> + */ >>>>>>> + void *opaque; >>>>>>> >>>>>> >>>>>> The corresponding AVFrame field is copied when copying props. >>>>>> >>>>> >>>>> Fixed both, thanks. Also copied the time_base field and made >>>>> av_init_packet() >>>>> initialize all fields. >>>>> Patch attached. Sorry it's taking me so long to work on this and >>>>> the Vulkan ABI changes. >>>>> >>>> >>>> Since no one yet has objected, will push this in 3 days unless >>>> someone does. >>>> >>> >>> Can you reply this this please? >>> >>> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html >>> >> >>> You wrote earlier that you don't like that you have to pass packets >>> to the >>> muxer in a timebase as set by the muxer's init function. Solving this by >>> adding a muxer open flag which saves the preferred time base of the user >>> and rescales all packets from the user's preferred time base to the real >>> time base before processing seems much more managable than >>> introducing the >>> AVPacket->time_base support everywhere and as far as I see it solves >>> this >>> problem just the same. >> >> I'm mainly introducing the field for myself. I have some (de)muxer >> code that's >> timestamp dependent, and timestamps don't make sense without knowing the >> time base. Since multiple streams go into that code, having a sideband >> way >> to plumb the timebases made the code very messy. The fact that they can >> also be used to save on an awkward and complicated mechanism to >> negotiate just comes as a bonus. Honestly, I had to read the mpv source >> code to realize that this is the correct sequence that has to happen, >> when none of this is even necessary. I'm sure other API users will >> find the >> field useful. >> >> I don't mind having a way to set the preferred time base, but I do >> think it'll >> be even more confusing if it converts time bases as well. We need a way >> to give a hint to muxers what time base you'd like, but I'd rather >> have it >> just remain a hint rather than also do the conversion, since it'll >> limit the >> usability to just your stream's input timebase. And if you have to >> specify your >> stream input timebase somewhere, then this would be better, where it's >> relevant. >> >>> Are there similar problems elsewhere? If there are, then is it not more >>> managable to allow the user to specify a preferred input or output >>> timebase during init instead of allowing per-packet timebases? By adding >>> time_base to AVPacket you basically say that it is possible >>> that each packet of a stream have a different time base. But in >>> realitly, >>> this never happens. >> >> We can add a comment saying this will never happen. Although if >> we do decide to allow packet timebase to dynamically change, >> and we add a similar field to frames (which I plan to do after this, >> but since >> we can add fields to AVFrames easily, I left it for later), then we >> would be >> able to dynamically handle more without effort from the user. >> For example, streams could be switched and concatenated in a way >> that doesn't break demuxing. elenril has some plans on writing a >> concat bsf, >> so he can say whether that could be useful there. > > OK, thanks. I can't say I am fully convinced, but apparently nobody > shares my concenrs, so feel free to go ahead with it. Others may have not paid attention to this thread just yet. This looks like a considerable change, so it probably could use other people's opinions. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lynne: > Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: > >> Lynne: >> >>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>> From: Lynne <dev@lynne.ee> >>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>> Subject: [PATCH] avpacket: ABI bump additions >>> >>> --- >>> libavcodec/avpacket.c | 5 +++++ >>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>> index e32c467586..03b73b3b53 100644 >>> --- a/libavcodec/avpacket.c >>> +++ b/libavcodec/avpacket.c >>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>> dst->flags = src->flags; >>> dst->stream_index = src->stream_index; >>> >>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>> + if (i < 0) >>> + return i; >>> >> >> 1. Don't use i here; add a new variable. >> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >> destination packet as uninitialized and make no attempt at unreferencing >> its content; yet you try to reuse opaque_ref. Even worse, you might >> return potentially dangerous packets: If the properties were >> uninitialized and av_packet_copy_props() failed, then the caller were >> not allowed to unreference the packet even when the non-properties were >> set to sane values. The easiest way to fix this is to move setting >> opaque ref to the place after initializing side_data below and either >> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >> av_buffer_replace(). It may also be best to unref it again if copying >> side data fails. >> >>> + >>> dst->side_data = NULL; >>> dst->side_data_elems = 0; >>> for (i = 0; i < src->side_data_elems; i++) { >>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>> void av_packet_unref(AVPacket *pkt) >>> { >>> av_packet_free_side_data(pkt); >>> + av_buffer_unref(&pkt->opaque_ref); >>> av_buffer_unref(&pkt->buf); >>> get_packet_defaults(pkt); >>> } >>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>> index fad8341c12..c29ad18a2b 100644 >>> --- a/libavcodec/packet.h >>> +++ b/libavcodec/packet.h >>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>> int64_t duration; >>> >>> int64_t pos; ///< byte position in stream, -1 if unknown >>> + >>> + /** >>> + * for some private data of the user >>> + */ >>> + void *opaque; >>> >> >> The corresponding AVFrame field is copied when copying props. >> > > Fixed both, thanks. Also copied the time_base field and made av_init_packet() > initialize all fields. Your new version is still not correct: If copying side data fails, the function returns without initializing opaque_ref at all, thereby making it dangerous to unref the packet (which e.g. av_packet_ref() does). - Andreas
James Almer: > On 7/7/2021 3:12 PM, Marton Balint wrote: >> >> >> On Wed, 7 Jul 2021, Lynne wrote: >> >>> 6 Jul 2021, 21:57 by cus@passwd.hu: >>> >>>> >>>> >>>> On Tue, 6 Jul 2021, Lynne wrote: >>>> >>>>> 3 Jun 2021, 06:58 by dev@lynne.ee: >>>>> >>>>>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>>>>> >>>>>>> Lynne: >>>>>>> >>>>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 >>>>>>>> 00:00:00 2001 >>>>>>>> From: Lynne <dev@lynne.ee> >>>>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>>>>> >>>>>>>> --- >>>>>>>> libavcodec/avpacket.c | 5 +++++ >>>>>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>>>>> 2 files changed, 26 insertions(+) >>>>>>>> >>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>>>> index e32c467586..03b73b3b53 100644 >>>>>>>> --- a/libavcodec/avpacket.c >>>>>>>> +++ b/libavcodec/avpacket.c >>>>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, >>>>>>>> const AVPacket *src) >>>>>>>> dst->flags = src->flags; >>>>>>>> dst->stream_index = src->stream_index; >>>>>>>> >>>>>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>>>>> + if (i < 0) >>>>>>>> + return i; >>>>>>>> >>>>>>> >>>>>>> 1. Don't use i here; add a new variable. >>>>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat >>>>>>> the >>>>>>> destination packet as uninitialized and make no attempt at >>>>>>> unreferencing >>>>>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>>>>> return potentially dangerous packets: If the properties were >>>>>>> uninitialized and av_packet_copy_props() failed, then the caller >>>>>>> were >>>>>>> not allowed to unreference the packet even when the >>>>>>> non-properties were >>>>>>> set to sane values. The easiest way to fix this is to move setting >>>>>>> opaque ref to the place after initializing side_data below and >>>>>>> either >>>>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>>>>> av_buffer_replace(). It may also be best to unref it again if >>>>>>> copying >>>>>>> side data fails. >>>>>>> >>>>>>>> + >>>>>>>> dst->side_data = NULL; >>>>>>>> dst->side_data_elems = 0; >>>>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, >>>>>>>> const AVPacket *src) >>>>>>>> void av_packet_unref(AVPacket *pkt) >>>>>>>> { >>>>>>>> av_packet_free_side_data(pkt); >>>>>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>>>>> av_buffer_unref(&pkt->buf); >>>>>>>> get_packet_defaults(pkt); >>>>>>>> } >>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>>>>> index fad8341c12..c29ad18a2b 100644 >>>>>>>> --- a/libavcodec/packet.h >>>>>>>> +++ b/libavcodec/packet.h >>>>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>>>>> int64_t duration; >>>>>>>> >>>>>>>> int64_t pos; ///< byte position in >>>>>>>> stream, -1 if unknown >>>>>>>> + >>>>>>>> + /** >>>>>>>> + * for some private data of the user >>>>>>>> + */ >>>>>>>> + void *opaque; >>>>>>>> >>>>>>> >>>>>>> The corresponding AVFrame field is copied when copying props. >>>>>>> >>>>>> >>>>>> Fixed both, thanks. Also copied the time_base field and made >>>>>> av_init_packet() >>>>>> initialize all fields. >>>>>> Patch attached. Sorry it's taking me so long to work on this and >>>>>> the Vulkan ABI changes. >>>>>> >>>>> >>>>> Since no one yet has objected, will push this in 3 days unless >>>>> someone does. >>>>> >>>> >>>> Can you reply this this please? >>>> >>>> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html >>>> >>> >>>> You wrote earlier that you don't like that you have to pass packets >>>> to the >>>> muxer in a timebase as set by the muxer's init function. Solving >>>> this by >>>> adding a muxer open flag which saves the preferred time base of the >>>> user >>>> and rescales all packets from the user's preferred time base to the >>>> real >>>> time base before processing seems much more managable than >>>> introducing the >>>> AVPacket->time_base support everywhere and as far as I see it solves >>>> this >>>> problem just the same. >>> >>> I'm mainly introducing the field for myself. I have some (de)muxer >>> code that's >>> timestamp dependent, and timestamps don't make sense without knowing the >>> time base. Since multiple streams go into that code, having a >>> sideband way >>> to plumb the timebases made the code very messy. The fact that they can >>> also be used to save on an awkward and complicated mechanism to >>> negotiate just comes as a bonus. Honestly, I had to read the mpv source >>> code to realize that this is the correct sequence that has to happen, >>> when none of this is even necessary. I'm sure other API users will >>> find the >>> field useful. >>> >>> I don't mind having a way to set the preferred time base, but I do >>> think it'll >>> be even more confusing if it converts time bases as well. We need a way >>> to give a hint to muxers what time base you'd like, but I'd rather >>> have it >>> just remain a hint rather than also do the conversion, since it'll >>> limit the >>> usability to just your stream's input timebase. And if you have to >>> specify your >>> stream input timebase somewhere, then this would be better, where it's >>> relevant. >>> >>>> Are there similar problems elsewhere? If there are, then is it not more >>>> managable to allow the user to specify a preferred input or output >>>> timebase during init instead of allowing per-packet timebases? By >>>> adding >>>> time_base to AVPacket you basically say that it is possible >>>> that each packet of a stream have a different time base. But in >>>> realitly, >>>> this never happens. >>> >>> We can add a comment saying this will never happen. Although if >>> we do decide to allow packet timebase to dynamically change, >>> and we add a similar field to frames (which I plan to do after this, >>> but since >>> we can add fields to AVFrames easily, I left it for later), then we >>> would be >>> able to dynamically handle more without effort from the user. >>> For example, streams could be switched and concatenated in a way >>> that doesn't break demuxing. elenril has some plans on writing a >>> concat bsf, >>> so he can say whether that could be useful there. >> >> OK, thanks. I can't say I am fully convinced, but apparently nobody >> shares my concenrs, so feel free to go ahead with it. > > Others may have not paid attention to this thread just yet. This looks > like a considerable change, so it probably could use other people's > opinions. > I am not really convinced either; and as soon as this field is there, users will (legitimately) expect it to be honoured by us; which is just not true as no demuxer sets it and all muxers and codecs ignore it. - Andreas
8 Jul 2021, 23:20 by andreas.rheinhardt@outlook.com: > James Almer: > >> On 7/7/2021 3:12 PM, Marton Balint wrote: >> >>> >>> >>> On Wed, 7 Jul 2021, Lynne wrote: >>> >>>> 6 Jul 2021, 21:57 by cus@passwd.hu: >>>> >>>>> >>>>> >>>>> On Tue, 6 Jul 2021, Lynne wrote: >>>>> >>>>>> 3 Jun 2021, 06:58 by dev@lynne.ee: >>>>>> >>>>>>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>>>>>> >>>>>>>> Lynne: >>>>>>>> >>>>>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 >>>>>>>>> 00:00:00 2001 >>>>>>>>> From: Lynne <dev@lynne.ee> >>>>>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>>>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>>>>>> >>>>>>>>> --- >>>>>>>>> libavcodec/avpacket.c | 5 +++++ >>>>>>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>>>>>> 2 files changed, 26 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>>>>> index e32c467586..03b73b3b53 100644 >>>>>>>>> --- a/libavcodec/avpacket.c >>>>>>>>> +++ b/libavcodec/avpacket.c >>>>>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, >>>>>>>>> const AVPacket *src) >>>>>>>>> dst->flags = src->flags; >>>>>>>>> dst->stream_index = src->stream_index; >>>>>>>>> >>>>>>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>>>>>> + if (i < 0) >>>>>>>>> + return i; >>>>>>>>> >>>>>>>> >>>>>>>> 1. Don't use i here; add a new variable. >>>>>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat >>>>>>>> the >>>>>>>> destination packet as uninitialized and make no attempt at >>>>>>>> unreferencing >>>>>>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>>>>>> return potentially dangerous packets: If the properties were >>>>>>>> uninitialized and av_packet_copy_props() failed, then the caller >>>>>>>> were >>>>>>>> not allowed to unreference the packet even when the >>>>>>>> non-properties were >>>>>>>> set to sane values. The easiest way to fix this is to move setting >>>>>>>> opaque ref to the place after initializing side_data below and >>>>>>>> either >>>>>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>>>>>> av_buffer_replace(). It may also be best to unref it again if >>>>>>>> copying >>>>>>>> side data fails. >>>>>>>> >>>>>>>>> + >>>>>>>>> dst->side_data = NULL; >>>>>>>>> dst->side_data_elems = 0; >>>>>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, >>>>>>>>> const AVPacket *src) >>>>>>>>> void av_packet_unref(AVPacket *pkt) >>>>>>>>> { >>>>>>>>> av_packet_free_side_data(pkt); >>>>>>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>>>>>> av_buffer_unref(&pkt->buf); >>>>>>>>> get_packet_defaults(pkt); >>>>>>>>> } >>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>>>>>> index fad8341c12..c29ad18a2b 100644 >>>>>>>>> --- a/libavcodec/packet.h >>>>>>>>> +++ b/libavcodec/packet.h >>>>>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>>>>>> int64_t duration; >>>>>>>>> >>>>>>>>> int64_t pos; ///< byte position in >>>>>>>>> stream, -1 if unknown >>>>>>>>> + >>>>>>>>> + /** >>>>>>>>> + * for some private data of the user >>>>>>>>> + */ >>>>>>>>> + void *opaque; >>>>>>>>> >>>>>>>> >>>>>>>> The corresponding AVFrame field is copied when copying props. >>>>>>>> >>>>>>> >>>>>>> Fixed both, thanks. Also copied the time_base field and made >>>>>>> av_init_packet() >>>>>>> initialize all fields. >>>>>>> Patch attached. Sorry it's taking me so long to work on this and >>>>>>> the Vulkan ABI changes. >>>>>>> >>>>>> >>>>>> Since no one yet has objected, will push this in 3 days unless >>>>>> someone does. >>>>>> >>>>> >>>>> Can you reply this this please? >>>>> >>>>> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-May/279855.html >>>>> >>>>> You wrote earlier that you don't like that you have to pass packets >>>>> to the >>>>> muxer in a timebase as set by the muxer's init function. Solving >>>>> this by >>>>> adding a muxer open flag which saves the preferred time base of the >>>>> user >>>>> and rescales all packets from the user's preferred time base to the >>>>> real >>>>> time base before processing seems much more managable than >>>>> introducing the >>>>> AVPacket->time_base support everywhere and as far as I see it solves >>>>> this >>>>> problem just the same. >>>>> >>>> >>>> I'm mainly introducing the field for myself. I have some (de)muxer >>>> code that's >>>> timestamp dependent, and timestamps don't make sense without knowing the >>>> time base. Since multiple streams go into that code, having a >>>> sideband way >>>> to plumb the timebases made the code very messy. The fact that they can >>>> also be used to save on an awkward and complicated mechanism to >>>> negotiate just comes as a bonus. Honestly, I had to read the mpv source >>>> code to realize that this is the correct sequence that has to happen, >>>> when none of this is even necessary. I'm sure other API users will >>>> find the >>>> field useful. >>>> >>>> I don't mind having a way to set the preferred time base, but I do >>>> think it'll >>>> be even more confusing if it converts time bases as well. We need a way >>>> to give a hint to muxers what time base you'd like, but I'd rather >>>> have it >>>> just remain a hint rather than also do the conversion, since it'll >>>> limit the >>>> usability to just your stream's input timebase. And if you have to >>>> specify your >>>> stream input timebase somewhere, then this would be better, where it's >>>> relevant. >>>> >>>>> Are there similar problems elsewhere? If there are, then is it not more >>>>> managable to allow the user to specify a preferred input or output >>>>> timebase during init instead of allowing per-packet timebases? By >>>>> adding >>>>> time_base to AVPacket you basically say that it is possible >>>>> that each packet of a stream have a different time base. But in >>>>> realitly, >>>>> this never happens. >>>>> >>>> >>>> We can add a comment saying this will never happen. Although if >>>> we do decide to allow packet timebase to dynamically change, >>>> and we add a similar field to frames (which I plan to do after this, >>>> but since >>>> we can add fields to AVFrames easily, I left it for later), then we >>>> would be >>>> able to dynamically handle more without effort from the user. >>>> For example, streams could be switched and concatenated in a way >>>> that doesn't break demuxing. elenril has some plans on writing a >>>> concat bsf, >>>> so he can say whether that could be useful there. >>>> >>> >>> OK, thanks. I can't say I am fully convinced, but apparently nobody >>> shares my concenrs, so feel free to go ahead with it. >>> >> >> Others may have not paid attention to this thread just yet. This looks >> like a considerable change, so it probably could use other people's >> opinions. >> > I am not really convinced either; and as soon as this field is there, > users will (legitimately) expect it to be honoured by us; which is just > not true as no demuxer sets it and all muxers and codecs ignore it. > I can change the patch to either initialize it as an invalid value (which would signal the user to instead get the timebase elsewhere) or set its value when the packet passes through the common demuxing function. Having this field in does not imply it's used during muxing at all, and in fact makes it easier to figure out how to properly mux since we can add the info to the comment (which we will have to, regardless of how we decide to implement the automatic method for timestamp rescaling, if we ever do decide to).
Quoting Lynne (2021-07-09 04:42:07) > > I can change the patch to either initialize it as an invalid value (which would > signal the user to instead get the timebase elsewhere) or set its value > when the packet passes through the common demuxing function. > Having this field in does not imply it's used during muxing at all, and in fact > makes it easier to figure out how to properly mux since we can add the info > to the comment (which we will have to, regardless of how we decide to implement > the automatic method for timestamp rescaling, if we ever do decide to). I am generally in favor of having the timebase in the packets, but there are tricky compatibility issues to consider. E.g. consider a case where a user: 1 reads packets from an encoder 2 rescales them to the muxer timebase manually 3 submits them to the muxer If we change encoders to set packet timebase and the muxers to honor it, then step 2 above will break. There are some ways around it (e.g. adding honor_packet_tb options to each component that deals with AVPackets, then gradually making them on by default, then removing the options), but it's a long game.
13 Jul 2021, 20:16 by anton@khirnov.net: > Quoting Lynne (2021-07-09 04:42:07) > >> >> I can change the patch to either initialize it as an invalid value (which would >> signal the user to instead get the timebase elsewhere) or set its value >> when the packet passes through the common demuxing function. >> Having this field in does not imply it's used during muxing at all, and in fact >> makes it easier to figure out how to properly mux since we can add the info >> to the comment (which we will have to, regardless of how we decide to implement >> the automatic method for timestamp rescaling, if we ever do decide to). >> > > I am generally in favor of having the timebase in the packets, but there > are tricky compatibility issues to consider. > > E.g. consider a case where a user: > 1 reads packets from an encoder > 2 rescales them to the muxer timebase manually > 3 submits them to the muxer > > If we change encoders to set packet timebase and the muxers to honor it, > then step 2 above will break. There are some ways around it (e.g. adding > honor_packet_tb options to each component that deals with AVPackets, > then gradually making them on by default, then removing the options), > but it's a long game. > We discussed this back in February, and I'm going for the long game, with a flag instead of an option. The flag won't be removed, there should still be an option for API users to not have our code touch timestamps. It'll just be made the default at some later bump, with a new flag introduced to restore old behaviour.
8 Jul 2021, 21:20 by andreas.rheinhardt@outlook.com: > Lynne: > >> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >> >>> Lynne: >>> >>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>> From: Lynne <dev@lynne.ee> >>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>> Subject: [PATCH] avpacket: ABI bump additions >>>> >>>> --- >>>> libavcodec/avpacket.c | 5 +++++ >>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>> 2 files changed, 26 insertions(+) >>>> >>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>> index e32c467586..03b73b3b53 100644 >>>> --- a/libavcodec/avpacket.c >>>> +++ b/libavcodec/avpacket.c >>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>> dst->flags = src->flags; >>>> dst->stream_index = src->stream_index; >>>> >>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>> + if (i < 0) >>>> + return i; >>>> >>> >>> 1. Don't use i here; add a new variable. >>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>> destination packet as uninitialized and make no attempt at unreferencing >>> its content; yet you try to reuse opaque_ref. Even worse, you might >>> return potentially dangerous packets: If the properties were >>> uninitialized and av_packet_copy_props() failed, then the caller were >>> not allowed to unreference the packet even when the non-properties were >>> set to sane values. The easiest way to fix this is to move setting >>> opaque ref to the place after initializing side_data below and either >>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>> av_buffer_replace(). It may also be best to unref it again if copying >>> side data fails. >>> >>>> + >>>> dst->side_data = NULL; >>>> dst->side_data_elems = 0; >>>> for (i = 0; i < src->side_data_elems; i++) { >>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>> void av_packet_unref(AVPacket *pkt) >>>> { >>>> av_packet_free_side_data(pkt); >>>> + av_buffer_unref(&pkt->opaque_ref); >>>> av_buffer_unref(&pkt->buf); >>>> get_packet_defaults(pkt); >>>> } >>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>> index fad8341c12..c29ad18a2b 100644 >>>> --- a/libavcodec/packet.h >>>> +++ b/libavcodec/packet.h >>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>> int64_t duration; >>>> >>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>> + >>>> + /** >>>> + * for some private data of the user >>>> + */ >>>> + void *opaque; >>>> >>> >>> The corresponding AVFrame field is copied when copying props. >>> >> >> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >> initialize all fields. >> > > Your new version is still not correct: If copying side data fails, the > function returns without initializing opaque_ref at all, thereby making > it dangerous to unref the packet (which e.g. av_packet_ref() does). > > - Andreas > Fixed, and made sure to unref it if copying side data fails.
20 Jul 2021, 00:05 by dev@lynne.ee: > 8 Jul 2021, 21:20 by andreas.rheinhardt@outlook.com: > >> Lynne: >> >>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>> >>>> Lynne: >>>> >>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>>> From: Lynne <dev@lynne.ee> >>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>> >>>>> --- >>>>> libavcodec/avpacket.c | 5 +++++ >>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>> 2 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>> index e32c467586..03b73b3b53 100644 >>>>> --- a/libavcodec/avpacket.c >>>>> +++ b/libavcodec/avpacket.c >>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>> dst->flags = src->flags; >>>>> dst->stream_index = src->stream_index; >>>>> >>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>> + if (i < 0) >>>>> + return i; >>>>> >>>> >>>> 1. Don't use i here; add a new variable. >>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>> destination packet as uninitialized and make no attempt at unreferencing >>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>> return potentially dangerous packets: If the properties were >>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>> not allowed to unreference the packet even when the non-properties were >>>> set to sane values. The easiest way to fix this is to move setting >>>> opaque ref to the place after initializing side_data below and either >>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>> av_buffer_replace(). It may also be best to unref it again if copying >>>> side data fails. >>>> >>>>> + >>>>> dst->side_data = NULL; >>>>> dst->side_data_elems = 0; >>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>> void av_packet_unref(AVPacket *pkt) >>>>> { >>>>> av_packet_free_side_data(pkt); >>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>> av_buffer_unref(&pkt->buf); >>>>> get_packet_defaults(pkt); >>>>> } >>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>> index fad8341c12..c29ad18a2b 100644 >>>>> --- a/libavcodec/packet.h >>>>> +++ b/libavcodec/packet.h >>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>> int64_t duration; >>>>> >>>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>>> + >>>>> + /** >>>>> + * for some private data of the user >>>>> + */ >>>>> + void *opaque; >>>>> >>>> >>>> The corresponding AVFrame field is copied when copying props. >>>> >>> >>> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >>> initialize all fields. >>> >> >> Your new version is still not correct: If copying side data fails, the >> function returns without initializing opaque_ref at all, thereby making >> it dangerous to unref the packet (which e.g. av_packet_ref() does). >> >> - Andreas >> > > Fixed, and made sure to unref it if copying side data fails. > Ping.
23 Jul 2021, 11:49 by dev@lynne.ee: > 20 Jul 2021, 00:05 by dev@lynne.ee: > >> 8 Jul 2021, 21:20 by andreas.rheinhardt@outlook.com: >> >>> Lynne: >>> >>>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>>> >>>>> Lynne: >>>>> >>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>>>> From: Lynne <dev@lynne.ee> >>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>>> >>>>>> --- >>>>>> libavcodec/avpacket.c | 5 +++++ >>>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>>> 2 files changed, 26 insertions(+) >>>>>> >>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>> index e32c467586..03b73b3b53 100644 >>>>>> --- a/libavcodec/avpacket.c >>>>>> +++ b/libavcodec/avpacket.c >>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>>> dst->flags = src->flags; >>>>>> dst->stream_index = src->stream_index; >>>>>> >>>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>>> + if (i < 0) >>>>>> + return i; >>>>>> >>>>> >>>>> 1. Don't use i here; add a new variable. >>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>>> destination packet as uninitialized and make no attempt at unreferencing >>>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>>> return potentially dangerous packets: If the properties were >>>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>>> not allowed to unreference the packet even when the non-properties were >>>>> set to sane values. The easiest way to fix this is to move setting >>>>> opaque ref to the place after initializing side_data below and either >>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>>> av_buffer_replace(). It may also be best to unref it again if copying >>>>> side data fails. >>>>> >>>>>> + >>>>>> dst->side_data = NULL; >>>>>> dst->side_data_elems = 0; >>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>>> void av_packet_unref(AVPacket *pkt) >>>>>> { >>>>>> av_packet_free_side_data(pkt); >>>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>>> av_buffer_unref(&pkt->buf); >>>>>> get_packet_defaults(pkt); >>>>>> } >>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>>> index fad8341c12..c29ad18a2b 100644 >>>>>> --- a/libavcodec/packet.h >>>>>> +++ b/libavcodec/packet.h >>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>>> int64_t duration; >>>>>> >>>>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>>>> + >>>>>> + /** >>>>>> + * for some private data of the user >>>>>> + */ >>>>>> + void *opaque; >>>>>> >>>>> >>>>> The corresponding AVFrame field is copied when copying props. >>>>> >>>> >>>> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >>>> initialize all fields. >>>> >>> >>> Your new version is still not correct: If copying side data fails, the >>> function returns without initializing opaque_ref at all, thereby making >>> it dangerous to unref the packet (which e.g. av_packet_ref() does). >>> >>> - Andreas >>> >> >> Fixed, and made sure to unref it if copying side data fails. >> > > Ping. > This has sat long enough, I'll push it tomorrow morning.
Lynne: > 8 Jul 2021, 21:20 by andreas.rheinhardt@outlook.com: > >> Lynne: >> >>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>> >>>> Lynne: >>>> >>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>>> From: Lynne <dev@lynne.ee> >>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>> >>>>> --- >>>>> libavcodec/avpacket.c | 5 +++++ >>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>> 2 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>> index e32c467586..03b73b3b53 100644 >>>>> --- a/libavcodec/avpacket.c >>>>> +++ b/libavcodec/avpacket.c >>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>> dst->flags = src->flags; >>>>> dst->stream_index = src->stream_index; >>>>> >>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>> + if (i < 0) >>>>> + return i; >>>>> >>>> >>>> 1. Don't use i here; add a new variable. >>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>> destination packet as uninitialized and make no attempt at unreferencing >>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>> return potentially dangerous packets: If the properties were >>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>> not allowed to unreference the packet even when the non-properties were >>>> set to sane values. The easiest way to fix this is to move setting >>>> opaque ref to the place after initializing side_data below and either >>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>> av_buffer_replace(). It may also be best to unref it again if copying >>>> side data fails. >>>> >>>>> + >>>>> dst->side_data = NULL; >>>>> dst->side_data_elems = 0; >>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>> void av_packet_unref(AVPacket *pkt) >>>>> { >>>>> av_packet_free_side_data(pkt); >>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>> av_buffer_unref(&pkt->buf); >>>>> get_packet_defaults(pkt); >>>>> } >>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>> index fad8341c12..c29ad18a2b 100644 >>>>> --- a/libavcodec/packet.h >>>>> +++ b/libavcodec/packet.h >>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>> int64_t duration; >>>>> >>>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>>> + >>>>> + /** >>>>> + * for some private data of the user >>>>> + */ >>>>> + void *opaque; >>>>> >>>> >>>> The corresponding AVFrame field is copied when copying props. >>>> >>> >>> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >>> initialize all fields. >>> >> >> Your new version is still not correct: If copying side data fails, the >> function returns without initializing opaque_ref at all, thereby making >> it dangerous to unref the packet (which e.g. av_packet_ref() does). >> >> - Andreas >> > > Fixed, and made sure to unref it if copying side data fails. > You indeed addressed the concerns I had; I still don't like the idea of having per-packet timebases (instead preferring Marton's approach), but it seems that we two are alone in this. So go ahead. - Andreas
2 Aug 2021, 05:03 by andreas.rheinhardt@outlook.com: > Lynne: > >> 8 Jul 2021, 21:20 by andreas.rheinhardt@outlook.com: >> >>> Lynne: >>> >>>> Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com: >>>> >>>>> Lynne: >>>>> >>>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001 >>>>>> From: Lynne <dev@lynne.ee> >>>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100 >>>>>> Subject: [PATCH] avpacket: ABI bump additions >>>>>> >>>>>> --- >>>>>> libavcodec/avpacket.c | 5 +++++ >>>>>> libavcodec/packet.h | 21 +++++++++++++++++++++ >>>>>> 2 files changed, 26 insertions(+) >>>>>> >>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>>>> index e32c467586..03b73b3b53 100644 >>>>>> --- a/libavcodec/avpacket.c >>>>>> +++ b/libavcodec/avpacket.c >>>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>>> dst->flags = src->flags; >>>>>> dst->stream_index = src->stream_index; >>>>>> >>>>>> + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); >>>>>> + if (i < 0) >>>>>> + return i; >>>>>> >>>>> >>>>> 1. Don't use i here; add a new variable. >>>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the >>>>> destination packet as uninitialized and make no attempt at unreferencing >>>>> its content; yet you try to reuse opaque_ref. Even worse, you might >>>>> return potentially dangerous packets: If the properties were >>>>> uninitialized and av_packet_copy_props() failed, then the caller were >>>>> not allowed to unreference the packet even when the non-properties were >>>>> set to sane values. The easiest way to fix this is to move setting >>>>> opaque ref to the place after initializing side_data below and either >>>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use >>>>> av_buffer_replace(). It may also be best to unref it again if copying >>>>> side data fails. >>>>> >>>>>> + >>>>>> dst->side_data = NULL; >>>>>> dst->side_data_elems = 0; >>>>>> for (i = 0; i < src->side_data_elems; i++) { >>>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) >>>>>> void av_packet_unref(AVPacket *pkt) >>>>>> { >>>>>> av_packet_free_side_data(pkt); >>>>>> + av_buffer_unref(&pkt->opaque_ref); >>>>>> av_buffer_unref(&pkt->buf); >>>>>> get_packet_defaults(pkt); >>>>>> } >>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >>>>>> index fad8341c12..c29ad18a2b 100644 >>>>>> --- a/libavcodec/packet.h >>>>>> +++ b/libavcodec/packet.h >>>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket { >>>>>> int64_t duration; >>>>>> >>>>>> int64_t pos; ///< byte position in stream, -1 if unknown >>>>>> + >>>>>> + /** >>>>>> + * for some private data of the user >>>>>> + */ >>>>>> + void *opaque; >>>>>> >>>>> >>>>> The corresponding AVFrame field is copied when copying props. >>>>> >>>> >>>> Fixed both, thanks. Also copied the time_base field and made av_init_packet() >>>> initialize all fields. >>>> >>> >>> Your new version is still not correct: If copying side data fails, the >>> function returns without initializing opaque_ref at all, thereby making >>> it dangerous to unref the packet (which e.g. av_packet_ref() does). >>> >>> - Andreas >>> >> >> Fixed, and made sure to unref it if copying side data fails. >> > You indeed addressed the concerns I had; I still don't like the idea of > having per-packet timebases (instead preferring Marton's approach), but > it seems that we two are alone in this. So go ahead. > > - Andreas > Thanks for reviewing it. Pushed!
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index e32c467586..03b73b3b53 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) dst->flags = src->flags; dst->stream_index = src->stream_index; + i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref); + if (i < 0) + return i; + dst->side_data = NULL; dst->side_data_elems = 0; for (i = 0; i < src->side_data_elems; i++) { @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src) void av_packet_unref(AVPacket *pkt) { av_packet_free_side_data(pkt); + av_buffer_unref(&pkt->opaque_ref); av_buffer_unref(&pkt->buf); get_packet_defaults(pkt); } diff --git a/libavcodec/packet.h b/libavcodec/packet.h index fad8341c12..c29ad18a2b 100644 --- a/libavcodec/packet.h +++ b/libavcodec/packet.h @@ -383,6 +383,27 @@ typedef struct AVPacket { int64_t duration; int64_t pos; ///< byte position in stream, -1 if unknown + + /** + * for some private data of the user + */ + void *opaque; + + /** + * AVBufferRef for free use by the API user. FFmpeg will never check the + * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when + * the packet is unreferenced. av_packet_copy_props() calls create a new + * reference with av_buffer_ref() for the target packet's opaque_ref field. + * + * This is unrelated to the opaque field, although it serves a similar + * purpose. + */ + AVBufferRef *opaque_ref; + + /** + * Time base of the packet's timestamps. + */ + AVRational time_base; } AVPacket; #if FF_API_INIT_PACKET