[FFmpeg-devel] RTP/SVQ3 payload parser

[Luca Abeni]

Hi Ronald,

is this the SV3Q/QDM2 that I had to review?
I finally found some time to have a look at it...


Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Aug 4, 2009 at 7:42 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
>> On Tue, Aug 04, 2009 at 06:00:42PM -0400, Ronald S. Bultje wrote:
>> [...]
>>> +/** return 0 on packet, no more left, 1 on packet, 1 on partial packet... */
>> am i the only one who has difficulty understanding this sentance?
>> also as we are already a this, maybe a enum would be better than 0,1,1
> 
> Attached is a fix for the typo, the last 1 would have to be a -1.
The "return 0 on packet, no more left, 1 on packet, -1 on partial packet..."
still looks a little bit cryptic to me... I would prefer a more explicit
"return: 0 if the packet has been fully parsed and a frame is completed,
< -1 in case of error, -1 if the packet has been parsed but no full frame
has been received, ..."
...". Moreover, does the sv3v_parse_packet() function really return 1 in
some case?

Moreover, it would be good to have a comment before
+    config_packet = buf[0] & 0x40;
+    start_packet  = buf[0] & 0x20;
+    end_packet    = buf[0] & 0x10;
explaining the meaning of the byte you are parsing here (BTW, you parse
one single byte, but increase buf by 2?).

[...]
+    if (!sv->pktbuf)
+        return -EINVAL;
+    put_buffer(sv->pktbuf, buf, len);
+
+    if (end_packet) {
+        av_init_packet(pkt);
+        pkt->stream_index = st->index;
+        pkt->pts          = sv->timestamp;
Are you sure about this? I think pkt->pts will be overwritten by
finalize_packet()... Shouldn't this be "*timestamp = sv->timestamp"?
Or maybe you do not even need to set anything... (*timestamp should
already contain the correct pts value).


+        sv->is_keyframe = flags & RTP_FLAG_KEY;
+    }
Who sets RTP_FLAG_KEY in flags?


Moreover, I have an additional question about this payload parser:
it seems to me that it is just buffering data until a frame is
complete (why isn't the "M" bit used?). This is not the same thing
done by all the other payload parsers, which just deliver the data
as soon as it arrives (without caring about returning complete
frames).
Why this difference? Is it because we do not have a parser for
CODEC_ID_SVQ3? If yes, a comment about this should be added, to
avoid confusion.

> In general, yes I'm in favour of an enum or some other form of
> providing better return values. One of the things I don't like about
> the current system for RTP is that I have to cache packets; I can't
> tell the caller to re-send the same (input) packet next time. What I
> would like:
[...]
>     return len;
> 
> }
> 
> Let me know what you think of this.

I have no particular opinions about this.


				Luca