[FFmpeg-devel] [PATCH] RTSP-MS 14/15: ASF packet parsing

[Michael Niedermayer]

On Sat, Apr 18, 2009 at 03:28:25PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Apr 18, 2009 at 2:56 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Sat, Apr 18, 2009 at 02:40:31PM -0400, Ronald S. Bultje wrote:
> >> On Sat, Apr 18, 2009 at 1:57 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> Oops, sorry. That last hack is still (only one this time) necessary, I
> >> was hoping to get to that next...
> [..]
> >> + ? ? ? ?else
> >> + ? ? ? ? ? ?return AVERROR_EOF;
> [..]
> > this was and is unacceptable
> 
> OK, I get that, but how do you want to do it then?
> 
> Before you continue, here's a description of the issue (also see
> bottom; hint, it works fine if it's a file!). Please read this.

please upload this file, i like to test if svn ffplay can play and
seek in it.


> 
> WMS payloads contain one ASF packet header, followed by the data. ASF
> files contain a series of these.
> asf_read_packet() does this, in essence:
> 
> int asf_read_packet()
> {
>   for (;;) {
>     get_packet_data(); // <- "ff_asf_parse_packet"
>     if (data or error or eof) return;
>     // we have extracted all data out of the current ASF packet
>     sync_to_new_packet(); // <- "ff_asf_get_packet"
>   }
> }
> 
> Looks good, right?

no, looks bad
thats a infinite loop, or at least it loops until EOF, which iam
pretty sure does not represent the asf demuxer


> 
> Here's why it fails for RTP payloads: we parse the first payload fine.
> Then, we finish the current packet and that's the EOF of the payload
> data. *HOWEVER*, that eof_reached of ByteIOContext is only set after
> we attempt to read one more byte, so it is not yet set. This is not
> the case for files, because there, EOF is computed based on the total
> ff_asf_data_header GUID. Therefore, it thinks it needs to go into the
> next ASF packet, and calls ff_asf_get_packet() for the second time in
> this payload, with the pointer of the input data being set exactly at
> the EOF point. If you look at ff_asf_get_packet(), you see it's a
> collection of get_byte() functions etc. w/o checks for input validity
> or EOF. What happens, as I've mentioned about 100 times now, is that
> every call returns 0 and eof_reached is now directly set. However, we
> don't ever check for EOF. Rather, we keep parsing. As a result, the
> demuxer thinks this is a regular packet and sets itself up for the
> next packet with random values for packet_length ("pkt_size_left")
> etc.. We re-enter parse_packet(), EOF is checked, confirmed and we
> return out of asf_read_packet().
> 
> Now, we load the next payload, but rather than parsing the header as
> we should, we directly start parsing the data, leading to nonsense
> outcome.
> 
> Please, please, please, read the above. Try to get it. I've explained
> it 10 times now. I don't want to explain it again. There's several
> ways to solve it, but try to first understand the problem.

you do not explain why you reach an invalid internal state (ake EOF
in the middle of the stream) you explain only that because the state
is not yet invalid the demuxer tries to contiunue and then it becomes
invalid (=EOF) and that you now want to break out but thats too late,

we first need to discuss WHY it becomes EOF and then either if
A. this can be avoided (does the demuxer read more than it needs, or
   does the RTP code not provide enough data for the ASF demuxer to
   return a single frame)
   its very unclear from your description if you pass enough data
   or not but my guess is you do not.
B. Change the ASF demuxer to support failing with EAGAIN on unavailable
   next packet, and before you attempt it no, you cannot treat url_feof()
   there by return EAGAIN that could end in an infinite loop if its real
   EOF

either way your eof check is broken and wrong, for example at the point
where you break out you could already have read 2 bytes from the next
packet successfully and i am not sure if the demuxer would recover from
that.


> 
> What my patch does is that it returns out of the parsing header when
> we're about to read these nonsense values. The subsequent call when
> the new payload is passed to the demuxer then works fine.
> 
> As I said, I'm sure there's other solutions, I'm open to them. But try
> to understand why I do this, what I'm trying to achieve.
> 
> > again, simple check, store all packets in a file, try to play it, try to
> > seek.
> > If it fails, your patchset is incomplete, and it WILL fail because the
> > written file will not have url_feof() == 1 after each packet
> 
> This works, because we're not setting nonsense values in any next call
> to ff_asf_get_packet(), because we don't reach EOS. Seeking fails,
> because "[asf @ 0x1032200]asf_read_pts failed". It probably means
> read_pts() doesn't actually sync, or the sync call isn't strict
> enough. That's fixable and implies a separate bug (i.e. asfdec.c
> assuming that the packetsize is always min/max_pktsize and that
> min_pktsize == max_pktsize, which is only true for regular files).

whatever the excuse if the demuxer cant seek the stream is not valid
or the demuxer is buggy.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090419/5b753234/attachment.pgp>