[FFmpeg-soc] [PATCH] qdm2 depacketizer

[Martin Storsjö]

Hi,

On Wed, 14 Jul 2010, Josh Allmann wrote:

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

True

> 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.

Yes we do, you just happened to remove the qdm->timestamp = -1 line :-)

> 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)

Hmm, that's weird.

A few more things - we're getting close.

> +        switch (config_item) {
> +            case 0: /* end of config block */
> +                return p - buf + item_len;
> +            case 1: /* stream without extradata */
> +                /* FIXME: set default qdm->block_size */
> +                break;
> +            case 2: /**< subpackets per block */
> +                if (item_len < 2)
> +                    return AVERROR_INVALIDDATA;

This needs to be item_len < 3, as I wrote in the previous mail, since 
you're reading p[2], which is the third byte.

> +                qdm->subpkts_per_block = p[2];
> +                break;
> +            case 3: /* superblock type */
> +                if (item_len < 4)
> +                    return AVERROR_INVALIDDATA;
> +                qdm->block_type = AV_RB16(p + 2);
> +                break;
> +            case 4: /* stream with extradata */
> +                if (item_len < 30)
> +                    return AVERROR_INVALIDDATA;
> +                av_freep(&st->codec->extradata);
> +                st->codec->extradata_size = 26 + item_len;
> +                if (!(st->codec->extradata = av_malloc(st->codec->extradata_size))){

You need to add the padding to the allocated area - but not as part of 
extradata_size. That is, av_mallocz(extradata_size + PADDING), so that the 
padding is zero-initialized, too.

> +                    st->codec->extradata_size = 0;
> +                    return AVERROR(ENOMEM);
> +                }


> +    if ((res = av_new_packet(pkt, qdm->block_size)) < 0)
> +        return res;
> +    memset(pkt->data, 0, pkt->size);
> +    pkt->pts           = qdm->timestamp;

This still isn't right. The original code did this:

> +    if (qdm->timestamp != (uint32_t) -1) {
> +        pkt->pts       = qdm->timestamp;
> +        qdm->timestamp = -1;
> +    } else
> +        pkt->pts       = AV_NOPTS_VALUE;

That is, store a valid pts only in the first packet, then nopts/-1 in the
rest, until qdm->timestamp is reinitialized.

You shouldn't write this into pkt->pts at all, doing that is an incorrect 
design that only will creep into more depacketizers if they use this as an 
example. pkt->pts gets set by the generic rtpdec code, and shouldn't be 
touched by the depacketizers. (Actually, this bug leads to pts values way 
off at the beginning, before the first RTCP packet, when the rtpdec code 
doesn't touch pkt->pts at all.)

So you probably need to pass the timestamp pointer into this
function, and then do something like this:

*timestamp = qdm->timestamp;
qdm->timestamp = RTP_NOTS_VALUE;

Or simply do that at the end of qdm2_parse_packet, that should be ok as 
well (and is a much smaller change compared to what you have now).

Also, you said:

> 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.

We reset qdm->timestamp after outputting it into the first packet, so that 
warrants doing it this way. Also, for the follow-up packets where no data 
has been provided in this call to _parse but we just return buffered data, 
we can't really rely on the value of *timestamp at all, I'd say.


So, after my rambling - remove the pkt->pts line, add qdm->timestamp = 
RTP_NOTS_VALUE after *timestamp = qdm->timestamp; - then it should be ok, 
with the minor changes I suggested above.

And since the changes that were left were quite minor, I went ahead and 
fixed them and applied it. Let's move on to other things. :-)

// Martin