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

[Ronald S. Bultje]

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.

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?

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.

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

Here's some potential questions that you could ask me:
- is that EOF "bug" in ff_asf_get_packet() also triggered for regular files?
No, because for regular files, we know the size of the complete file
(ff_asf_data_header size), and thus enter EOS state one step earlier,
at the end of the last frame of the last ASF packet
- so why don't you create a "fillable" ByteIOContext in rtp_asf.c?
Seems complicated... I can do if you wish, no idea how though, it
seems like it'd involve a lot of threading or crazy stuff...
- why don't you fill two payloads into ByteIOContext?
Again, seems complicated, because you get extended caching at multiple
levels... Especially if you think video, where one frame could be
spread over multiple packets already and I'd thus have to read several
tens of payloads ahead to be safe... Seems crazy to me.

Ronald