[FFmpeg-soc] [PATCH] qdm2 depacketizer

[Josh Allmann]

On 14 July 2010 05:33, Martin Storsjö <martin at martin.st> wrote:
> Hi,
>
> On Wed, 14 Jul 2010, Josh Allmann wrote:
>
>> 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 :-)
>

D'oh. That clears up things.

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

I wonder how many stupid bugs can be attributed to coding at 3am.

[...]
>> +    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:

I could've sworn I removed that line. Oh well.


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

Gotcha -- it all makes sense now when we re-set qdm->timestamp.

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

\o/
packetizers!

Josh