[FFmpeg-soc] [PATCH] qdm2 depacketizer

[Martin Storsjö]

On Thu, 8 Jul 2010, Ronald S. Bultje wrote:

> On Thu, Jul 8, 2010 at 4:02 AM, Martin Storsjö <martin at martin.st> 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

if (foo)
   pkt->pts = qdm->timestamp;
else
   pkt->pts = AV_NOPTS_VALUE;

into

if (foo)
   *timestamp = qdm->timestamp;
else
   *timestamp = RTP_NOTS_VALUE;

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Allow-depacketizers-to-specify-that-pkt-pts-should-b.patch
Type: text/x-diff
Size: 2235 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100708/469eb3bb/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-rtpdec_svq3-Return-the-timestamp-in-timestamp-instea.patch
Type: text/x-diff
Size: 1218 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100708/469eb3bb/attachment-0001.patch>