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

[Martin Storsjö]

On Wed, 13 Oct 2010, Luca Barbato wrote:

> On 10/12/2010 11:48 PM, Ronald S. Bultje wrote:
> > 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. :-).
> 
> +1

Thanks, applied these, plus another minor trivial fix for an issue I 
found.

As for the name prev_ret, we could change it to 
"prev_packet_has_more_data" or something similar, if something like that 
looks less ugly to you.

// Martin