| Message ID | 1686953578-18843-2-git-send-email-dheitmueller@ltnglobal.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add passthrough support for SCTE-35 | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Devin Heitmueller: > In order to properly process SCTE-35 packets, we need the original > PTS value from the demux (i.e. not mangled by the application or > reclocked for the output). This allows us to set the pts_adjustment > field in an BSF on the output side. > > Introduce a new side data type to store the original PTS. > > Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> > --- > libavcodec/packet.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/libavcodec/packet.h b/libavcodec/packet.h > index f28e7e7..a86a550 100644 > --- a/libavcodec/packet.h > +++ b/libavcodec/packet.h > @@ -300,6 +300,16 @@ enum AVPacketSideDataType { > AV_PKT_DATA_DYNAMIC_HDR10_PLUS, > > /** > + * Provides the original PTS when passed through the demux. This can > + * be used to offset any subsequent changes made by the caller to > + * adjust PTS values (such as pts_offset). We need this for SCTE-35, > + * since by the time the packets reach the output the PTS values have > + * already been re-written, and we cannot calculate pre-roll values > + * using the PTS values embedded in the packet content > + */ > + AV_PKT_DATA_ORIG_PTS, > + > + /** > * The number of side data types. > * This is not part of the public API/ABI in the sense that it may > * change when new side data types are added. A timestamp without a timebase? Doesn't sound good to me. And it also seems quite hacky. Apart from that: It needs to specify that the data is a int64_t. - Andreas
Hi Andreas, Thanks for the feedback. I put out an RFC back in March but got no comments. On Fri, Jun 16, 2023 at 6:01 PM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > A timestamp without a timebase? Doesn't sound good to me. And it also > seems quite hacky. > Apart from that: It needs to specify that the data is a int64_t. So you're suggesting a struct that contains both the timestamp and a timebase? I don't have any real objection to this. I agree it seems hacky, but don't have a better solution. I welcome constructive suggestions. I had considered using an AVPacket metadata field rather than a new side data type (as that won't necessarily lock us into a new side data type that we would have to support), and the functionality is really specific to one use case. However I figured side data might be better since it avoids the conversion of the PTS to a string and back. Devin
Quoting Devin Heitmueller (2023-06-19 15:14:38) > Hi Andreas, > > Thanks for the feedback. I put out an RFC back in March but got no comments. > > On Fri, Jun 16, 2023 at 6:01 PM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: > > A timestamp without a timebase? Doesn't sound good to me. And it also > > seems quite hacky. > > Apart from that: It needs to specify that the data is a int64_t. > > So you're suggesting a struct that contains both the timestamp and a > timebase? I don't have any real objection to this. > > I agree it seems hacky, but don't have a better solution. I welcome > constructive suggestions. I had considered using an AVPacket metadata > field rather than a new side data type (as that won't necessarily lock > us into a new side data type that we would have to support), This reasoning is backwards IMO. You'd be creating an implicit API while pretending not to. If anyone uses it, they would expect stability, otherwise what's the point. > functionality is really specific to one use case. However I figured > side data might be better since it avoids the conversion of the PTS to > a string and back. One one hand it does feel hacky, on the other I can see valid uses for it. Though 'orig' is rather vague, I'd call it something like 'transport timestamp' instead. A timebase should definitely be bundled with it.
On Mon, Jun 19, 2023 at 10:32 AM Anton Khirnov <anton@khirnov.net> wrote: > This reasoning is backwards IMO. You'd be creating an implicit API while > pretending not to. If anyone uses it, they would expect stability, > otherwise what's the point. My concern was that given right now it only has a single use case that we might not have thought of other ways it may be used in the future, so using a metadata field might allow us to keep it "semi-private" until others wanted the functionality. My fear was that we wouldn't be able to reach a consensus on a new public API definition that we would be willing to support indefinitely. > > functionality is really specific to one use case. However I figured > > side data might be better since it avoids the conversion of the PTS to > > a string and back. > > One one hand it does feel hacky, on the other I can see valid uses for > it. Though 'orig' is rather vague, I'd call it something like 'transport > timestamp' instead. A timebase should definitely be bundled with it. I don't feel strongly about the naming. My goal was to convey that this was the original input timestamp on arrival, and not something that has been transformed, and I worry that "transport" might be a bit vague as it isn't clear whether this was on input, output, or somewhere else in the workflow. That said, pick a name that will be considered acceptable to be merged upstream, and I'll adjust the patch accordingly. Devin
diff --git a/libavcodec/packet.h b/libavcodec/packet.h index f28e7e7..a86a550 100644 --- a/libavcodec/packet.h +++ b/libavcodec/packet.h @@ -300,6 +300,16 @@ enum AVPacketSideDataType { AV_PKT_DATA_DYNAMIC_HDR10_PLUS, /** + * Provides the original PTS when passed through the demux. This can + * be used to offset any subsequent changes made by the caller to + * adjust PTS values (such as pts_offset). We need this for SCTE-35, + * since by the time the packets reach the output the PTS values have + * already been re-written, and we cannot calculate pre-roll values + * using the PTS values embedded in the packet content + */ + AV_PKT_DATA_ORIG_PTS, + + /** * The number of side data types. * This is not part of the public API/ABI in the sense that it may * change when new side data types are added.
In order to properly process SCTE-35 packets, we need the original PTS value from the demux (i.e. not mangled by the application or reclocked for the output). This allows us to set the pts_adjustment field in an BSF on the output side. Introduce a new side data type to store the original PTS. Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> --- libavcodec/packet.h | 10 ++++++++++ 1 file changed, 10 insertions(+)