| Message ID | 1690810686-4723-3-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 |
On Mon, 31 Jul 2023 at 09:38, Devin Heitmueller < devin.heitmueller@ltnglobal.com> wrote: > We need the original PTS value in order to do subsequent processing, > so set it as packet side data. > > Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> > --- > libavformat/mpegts.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index 0b3edda..a1b2420 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -1783,8 +1783,17 @@ static void scte_data_cb(MpegTSFilter *filter, > const uint8_t *section, > prg = av_find_program_from_stream(ts->stream, NULL, idx); > if (prg && prg->pcr_pid != -1 && prg->discard != AVDISCARD_ALL) { > MpegTSFilter *f = ts->pids[prg->pcr_pid]; > - if (f && f->last_pcr != -1) > + if (f && f->last_pcr != -1) { > + AVTransportTimestamp *transport_ts; > ts->pkt->pts = ts->pkt->dts = f->last_pcr/300; > + transport_ts = (AVTransportTimestamp *) > av_packet_new_side_data(ts->pkt, > + > AV_PKT_DATA_TRANSPORT_TIMESTAMP, > + > sizeof(AVTransportTimestamp)); > + if (transport_ts) { > + transport_ts->pts = ts->pkt->pts; > + transport_ts->time_base = av_make_q(1, 90000); > + } > + } > } > ts->stop_parse = 1; > How is this frame accurate? Surely "last_pcr" can be up to 100ms out. You need to actually be interpolating the true value in order to be frame accurate (not saying this is easy/doable in FFmpeg). But at the same time inaccurate splices aren't great either. Kieran
Hi Kieran,
Thanks for your review.
On Wed, Aug 9, 2023 at 9:55 AM Kieran Kunhya <kierank@obe.tv> wrote:
> How is this frame accurate? Surely "last_pcr" can be up to 100ms out. You need to actually be interpolating the true value in order to be frame accurate (not saying this is easy/doable in FFmpeg). But at the same time inaccurate splices aren't great either.
So it's worth noting that the patch I've proposed doesn't change the
existing logic in terms of how the timestamp is determined. The patch
in question simply makes the existing timestamp available as side
data.
Second, in most cases the accuracy of the timestamp for the SCTE
message isn't actually that important for frame accuracy. It's the
splice time that is important (usually specified as a PTS). And hence
even if the timestamp of the SCTE message is off by a bit you can
still have frame accurate splicing.
Now it's true that the splice-immediate case does benefit by the value
being more accurate. I have a separate patch which better tracks the
video PTS and uses that as the basis for specifying the SCTE-35
timestamp value (and that's what I use in production). I will be
looking to submit that as a separate patch, but didn't want to muddy
the waters by introducing it in this patch series (where I'm not
trying to tackle that problem).
In short, this patch series does significantly improve the situation,
even though it doesn't attempt to tackle the problem of the SCTE-35
timestamp not being as accurate as it could be.
Devin
--
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com e: devin.heitmueller@ltnglobal.com
On Wed, 9 Aug 2023, 12:37 Devin Heitmueller, < devin.heitmueller@ltnglobal.com> wrote: > eI have a separate patch which better tracks the > video PTS and uses that as the basis for specifying the SCTE-35 > timestamp value (and that's what I use in production). I will be > looking to submit that as a separate patch, but didn't want to muddy > the waters by introducing it in this patch series (where I'm not > trying to tackle that problem). > The (closest?) video PTS is even worse than the last PCR because the VBV means the closest PTS can be quite far from the interpolated PCR. Kieran >
On Thu, Aug 10, 2023 at 8:13 AM Kieran Kunhya <kierank@obe.tv> wrote:
> The (closest?) video PTS is even worse than the last PCR because the VBV means the closest PTS can be quite far from the interpolated PCR.
It's arguments like that which prompted me to explicitly exclude such
a patch from the series. It's a discussion to be had, but not
relevant for this patch series (which makes no effort to change the
logic for how the timestamp is determined).
Wait until such a patch is submitted, and then we can debate at length
the ambiguity in the specification and what the best approach is.
Thanks,
Devin
On Thu, 10 Aug 2023 at 08:20, Devin Heitmueller < devin.heitmueller@ltnglobal.com> wrote: > On Thu, Aug 10, 2023 at 8:13 AM Kieran Kunhya <kierank@obe.tv> wrote: > > The (closest?) video PTS is even worse than the last PCR because the VBV > means the closest PTS can be quite far from the interpolated PCR. > > It's arguments like that which prompted me to explicitly exclude such > a patch from the series. It's a discussion to be had, but not > relevant for this patch series (which makes no effort to change the > logic for how the timestamp is determined). > > Wait until such a patch is submitted, and then we can debate at length > the ambiguity in the specification and what the best approach is. > There is zero ambiguity in the specification. Kieran
On Thu, 10 Aug 2023 at 08:41, Kieran Kunhya <kierank@obe.tv> wrote: > On Thu, 10 Aug 2023 at 08:20, Devin Heitmueller < > devin.heitmueller@ltnglobal.com> wrote: > >> On Thu, Aug 10, 2023 at 8:13 AM Kieran Kunhya <kierank@obe.tv> wrote: >> > The (closest?) video PTS is even worse than the last PCR because the >> VBV means the closest PTS can be quite far from the interpolated PCR. >> >> It's arguments like that which prompted me to explicitly exclude such >> a patch from the series. It's a discussion to be had, but not >> relevant for this patch series (which makes no effort to change the >> logic for how the timestamp is determined). >> >> Wait until such a patch is submitted, and then we can debate at length >> the ambiguity in the specification and what the best approach is. >> > > There is zero ambiguity in the specification. > Like any other form of SI in MPEG-TS the timestamp (although there is actually no such thing) is "now", which by definition is the interpolated PCR. Taking a video frame PTS is incorrect. What's the point of submitting patches like this exposing things in the public API that you know to be wrong? Kieran
On Thu, Aug 10, 2023 at 8:48 AM Kieran Kunhya <kierank@obe.tv> wrote: > > > > On Thu, 10 Aug 2023 at 08:41, Kieran Kunhya <kierank@obe.tv> wrote: >> >> On Thu, 10 Aug 2023 at 08:20, Devin Heitmueller <devin.heitmueller@ltnglobal.com> wrote: >>> >>> On Thu, Aug 10, 2023 at 8:13 AM Kieran Kunhya <kierank@obe.tv> wrote: >>> > The (closest?) video PTS is even worse than the last PCR because the VBV means the closest PTS can be quite far from the interpolated PCR. >>> >>> It's arguments like that which prompted me to explicitly exclude such >>> a patch from the series. It's a discussion to be had, but not >>> relevant for this patch series (which makes no effort to change the >>> logic for how the timestamp is determined). >>> >>> Wait until such a patch is submitted, and then we can debate at length >>> the ambiguity in the specification and what the best approach is. >> >> >> There is zero ambiguity in the specification. > > > Like any other form of SI in MPEG-TS the timestamp (although there is actually no such thing) is "now", which by definition is the interpolated PCR. > Taking a video frame PTS is incorrect. > > What's the point of submitting patches like this exposing things in the public API that you know to be wrong? Again, this patch series makes no attempt to address the problem you are complaining about. It brings the situation from "completely doesn't work" to "works fine the majority of the time except for the splice immediate case where the timestamp may not be as accurate as it could be". And let's be fair, splice immediate is both an uncommon use case in the industry and nobody doing a splice immediate expects it to be frame accurate (as it's typically initiated by a human during live programming). I'm happy to have this discussion, but it doesn't have any bearing on whether this patch series should be accepted. Let's not throw out the baby with the bathwater. Devin
On Thu, 10 Aug 2023, 08:59 Devin Heitmueller, < devin.heitmueller@ltnglobal.com> wrote: > On Thu, Aug 10, 2023 at 8:48 AM Kieran Kunhya <kierank@obe.tv> wrote: > > > > > > > > On Thu, 10 Aug 2023 at 08:41, Kieran Kunhya <kierank@obe.tv> wrote: > >> > >> On Thu, 10 Aug 2023 at 08:20, Devin Heitmueller < > devin.heitmueller@ltnglobal.com> wrote: > >>> > >>> On Thu, Aug 10, 2023 at 8:13 AM Kieran Kunhya <kierank@obe.tv> wrote: > >>> > The (closest?) video PTS is even worse than the last PCR because the > VBV means the closest PTS can be quite far from the interpolated PCR. > >>> > >>> It's arguments like that which prompted me to explicitly exclude such > >>> a patch from the series. It's a discussion to be had, but not > >>> relevant for this patch series (which makes no effort to change the > >>> logic for how the timestamp is determined). > >>> > >>> Wait until such a patch is submitted, and then we can debate at length > >>> the ambiguity in the specification and what the best approach is. > >> > >> > >> There is zero ambiguity in the specification. > > > > > > Like any other form of SI in MPEG-TS the timestamp (although there is > actually no such thing) is "now", which by definition is the interpolated > PCR. > > Taking a video frame PTS is incorrect. > > > > What's the point of submitting patches like this exposing things in the > public API that you know to be wrong? > > Again, this patch series makes no attempt to address the problem you > are complaining about. It brings the situation from "completely > doesn't work" to "works fine the majority of the time except for the > splice immediate case where the timestamp may not be as accurate as it > could be". And let's be fair, splice immediate is both an uncommon > use case in the industry and nobody doing a splice immediate expects > it to be frame accurate (as it's typically initiated by a human during > live programming). > > I'm happy to have this discussion, but it doesn't have any bearing on > whether this patch series should be accepted. Let's not throw out the > baby with the bathwater. > You're exposing this incorrect information as public API, two wrongs don't make a right. I also told the author of the previous code that it was wrong but the patch was forced through on the guise that "professionals won't respect ffmpeg if scte-35 isn't demuxed". The fact something isn't used often, doesn't mean it should be implemented badly. You could say that interlaced isn't used much as a total of all the video in the world so we should just not decode it correctly. By all means keep your hacks in your forks. Kieran >
Hi Kieran, On Thu, Aug 10, 2023 at 9:09 AM Kieran Kunhya <kierank@obe.tv> wrote: > You're exposing this incorrect information as public API, two wrongs don't make a right. The information is already exposed via the public API, in the form of pkt->pts. The fact that I've exposed that same value as side data as well doesn't really change that situation. > I also told the author of the previous code that it was wrong but the patch was forced through on the guise that "professionals won't respect ffmpeg if scte-35 isn't demuxed". So you're annoyed that seven years ago somebody else submitted some patch and it got merged over your objection? And that in that time nobody (including you) has cared enough to take it upon themselves to improve the accuracy of the timestamp? > The fact something isn't used often, doesn't mean it should be implemented badly. You could say that interlaced isn't used much as a total of all the video in the world so we should just not decode it correctly. I've made no attempt to argue that the accuracy of the timestamp couldn't be improved. I would argue that improving it shouldn't be any sort of prerequisite to merging this patch series though. Presumably it hasn't happened already because 1) properly implementing PCR interpolation within the ffmpeg mpegts demux implementation is difficult, 2) nobody has cared enough to do it, and/or 3) the typical PCR interval is 40ms so people are happy with it being "close enough" in the splice immediate case. > By all means keep your hacks in your forks. Where is the hack? My patches are an incremental improvement over what is already there. The notion that it makes no attempt to solve some orthogonal problem that I'm not really worried about isn't grounds for rejecting it. I would be happy to see other developers weigh in, or if Kieran's is the final word then I would like to see this reviewed by the TC. I'm making a genuine effort to improve the support in ffmpeg and it seems like an artificial barrier is being raised against those efforts. Devin Devin -- Devin Heitmueller, Senior Software Engineer LTN Global Communications o: +1 (301) 363-1001 w: https://ltnglobal.com e: devin.heitmueller@ltnglobal.com
Sent from my mobile device On Thu, 10 Aug 2023, 11:12 Devin Heitmueller, < devin.heitmueller@ltnglobal.com> wrote: > Hi Kieran, > > On Thu, Aug 10, 2023 at 9:09 AM Kieran Kunhya <kierank@obe.tv> wrote: > > You're exposing this incorrect information as public API, two wrongs > don't make a right. > > The information is already exposed via the public API, in the form of > pkt->pts. The fact that I've exposed that same value as side data as > well doesn't really change that situation. > The whole exposing of SI as if it were PES is a giant hack in general, SI doesn't have a PTS. Kieran
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index 0b3edda..a1b2420 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -1783,8 +1783,17 @@ static void scte_data_cb(MpegTSFilter *filter, const uint8_t *section, prg = av_find_program_from_stream(ts->stream, NULL, idx); if (prg && prg->pcr_pid != -1 && prg->discard != AVDISCARD_ALL) { MpegTSFilter *f = ts->pids[prg->pcr_pid]; - if (f && f->last_pcr != -1) + if (f && f->last_pcr != -1) { + AVTransportTimestamp *transport_ts; ts->pkt->pts = ts->pkt->dts = f->last_pcr/300; + transport_ts = (AVTransportTimestamp *) av_packet_new_side_data(ts->pkt, + AV_PKT_DATA_TRANSPORT_TIMESTAMP, + sizeof(AVTransportTimestamp)); + if (transport_ts) { + transport_ts->pts = ts->pkt->pts; + transport_ts->time_base = av_make_q(1, 90000); + } + } } ts->stop_parse = 1;
We need the original PTS value in order to do subsequent processing, so set it as packet side data. Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> --- libavformat/mpegts.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)