[FFmpeg-soc] [PATCH] qdm2 depacketizer

[Josh Allmann]

Hi,

On 8 July 2010 12:22, Martin Storsjö <martin at martin.st> wrote:
> 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;
>

In this case, foo is qdm->timestamp != RTP_NOTS_VALUE.
That makes the whole hunk tautologous, so simplified to
*timestamp = qdm->timestamp.

Although the whole thing still feels a bit odd, as we assign
qdm->timestamp = *timestamp earlier in the code, and don't use
qdm->timestamp otherwise.

I also removed the pkt->pts code, as that doesn't appear to be used.

Fixes have also been made for all earlier comments, with Martin's
patches applied locally. (Just a heads up -- git complains about ^M
(DOS-style?) line endings in those)

Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: qdm2.diff
Type: text/x-patch
Size: 13965 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100714/ef300e94/attachment.bin>