[FFmpeg-devel] [PATCH] rtpdec: Better logic for immediately returning packets from the queue

[Ronald S. Bultje]

Hi,

On Tue, Oct 12, 2010 at 5:44 PM, Martin Storsj? <martin at martin.st> wrote:
> On Sat, 9 Oct 2010, Martin Storsj? wrote:
>> On Fri, 8 Oct 2010, Martin Storsj? wrote:
>> > On Fri, 8 Oct 2010, Luca Barbato wrote:
>> > > On 10/08/2010 11:44 AM, Martin Storsj? wrote:
>> > > > On Fri, 8 Oct 2010, Luca Barbato wrote:
>> > > >> On 10/08/2010 11:16 AM, Martin Storsj? wrote:
>> > > >>> But what if packets 1-5 were one single large fragmented packet? Then the
>> > > >>> depacketization of packet 1 would return AVERROR(EAGAIN), and return no
>> > > >>> data until the whole frame has been depacketized. Currently, we'd return
>> > > >>> the control to the caller, and only proceed to check packet 2 the next
>> > > >>> time we're called. These patches instead make sure we check if we have the
>> > > >>> next packet in sequence, and try to parse that, if the current parsing
>> > > >>> returned< ? 0.
>> > > >>>
>> > > >>> I've tested this with the VP8 depacketizer that uses this logic, with an
>> > > >>> intentionally scrambled/reordered sender, and it seems to work as I intend
>> > > >>> now.
>> > > >>>
>> > > >>> Opinions?
>> > > >>
>> > > >> I'd check for EAGAIN explicitly, beside that it's fine for me.
>> > > >
>> > > > Not all of them return AVERROR(EAGAIN), some just return -1, too...
>> > >
>> > > That might need attention.
>> >
>> > True, and being able to return data sooner in this scenario is a good
>> > incentive for returning proper error codes in the depacketizers.
>> >
>> > > > And on
>> > > > the other side, even if packet 1 failed due to it being invalid, I still
>> > > > think we can try to depacketize the next one if we have it, and see if
>> > > > that one succeeds instead.
>> > >
>> > > I think would be better notify this to the upper layer sooner and not
>> > > hide it.
>> >
>> > Fair enough. Like the attached, then?
>>
>> Updated series attached, achieving the same thing, but in a slightly
>> cleaner way. This avoids having to manually store prev_ret at every return
>> statement in the code, which can easily be forgotten, by doing this in an
>> outer function.
>
> Ping - Luca B and Ronald, does this look sane?

Looks good to me. I keep thinking that prev_ret is a little ugly but
I'll live with it for now. :-).

Ronald