On 8 July 2010 12:22, Martin Storsjö <[email protected]> wrote: > On Thu, 8 Jul 2010, Ronald S. Bultje wrote: > >> On Thu, Jul 8, 2010 at 4:02 AM, Martin Storsjö <[email protected]> wrote: >> > On Wed, 7 Jul 2010, Josh Allmann wrote: >> >> + if (qdm->timestamp != (uint32_t) -1) { >> >> + pkt->pts = qdm->timestamp; >> >> + qdm->timestamp = -1; >> >> + } else >> >> + pkt->pts = AV_NOPTS_VALUE; >> > >> > This isn't right. (Now I see that something similar slipped through in the >> > svq3 patch, too, but with less impact.) >> > >> > pkt->pts is set (in this case, overwritten) by finalize_packet in >> > rtpdec.c, where it calculates a proper timestamp using RTCP sync and all >> > that. >> > >> > If we want to base that calculation on another RTP timestamp, we need to >> > return the timestamp we want via *timestamp qdm2_parse_packet, so that >> > timestamp is used in the RTP timestamp -> realtime timestamp calculation >> > in finalize_packet. >> > >> > Also, for the follow-up cases, where no data is provided to >> > qdm2_parse_packet but we return some already buffered data, lines 411-415 >> > in rtpdec.c, we don't know the RTP timestamp, so it is left at 0 unless >> > the depacketizer overwrites it. >> > >> > The problematic case is if we explicitly want to set pkt->pts to >> > AV_NOPTS_VALUE, since the timestamp pointer/value is an uint32_t, and we >> > can't pass an AV_NOPTS_VALUE there. We could introduce an (uint32_t)-1, >> > which would be interpreted as "don't set pkt->pts". >> >> I think I wrote this to ensure timestamps are increasing (rather than >> staying the same) when a single RTP packet contains multiple QDM2 >> frames. Feel free to solve this in any way you think is best, this is >> admittedly a hack. >> >> I haven't looked at the SVQ3 code to see what's up there, but I think >> we shouldn't have to do anything unless you can have multiple SVQ3 >> frames in one RTP packet. Timestamp code there can likely be removed >> with no side-effects... > > Yes, it shouldn't really be necessary at all there, but setting it (in a > corrected way) may be more correct in some scenarios with dropped packets. > > The attached patches allows depacketizers to specify that they want > AV_NOPTS_VALUE by returning RTP_NOTS_VALUE in *timestamp, and fix up the > svq3 depacketizers to handle timestamps correctly. > > Josh, when/if we apply this, you should be able to change the code that > does >
Those patches look good; I'll apply them locally regardless and work from there. Josh > if (foo) > pkt->pts = qdm->timestamp; > else > pkt->pts = AV_NOPTS_VALUE; > > into > > if (foo) > *timestamp = qdm->timestamp; > else > *timestamp = RTP_NOTS_VALUE; > > // Martin > _______________________________________________ > FFmpeg-soc mailing list > [email protected] > https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc > > _______________________________________________ FFmpeg-soc mailing list [email protected] https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
