[FFmpeg-devel] [RFC/PATCH] Pass PRIVATE_STREAM_2 MPEG-PS packets to caller

[Richard]

On 15/03/13 21:50, Michael Niedermayer wrote:
> On Fri, Mar 15, 2013 at 11:28:03AM +0100, Richard wrote:
>> On 15/03/13 00:10, Michael Niedermayer wrote:
>>> On Fri, Mar 08, 2013 at 09:41:50PM +0100, Richard wrote:
>>>> +
>>>> +            while (len >= 6) {
>>>> +                len--;
>>>>                   if (avio_r8(s->pb) == 'S') {
>>>>                       uint8_t buf[5];
>>>>                       avio_read(s->pb, buf, sizeof(buf));
>>>> @@ -259,9 +269,32 @@ static int mpegps_read_pes_header(AVFormatContext *s,
>>>>                   }
>>>>               }
>>>>               m->sofdec -= !m->sofdec;
>>>
>>> you change sofdec detection behavior here
>>> before it run once, after your patch it runs on every packet
>>
>> No it doesn't.  There's still the check for '!m->sofdec', it's just
>> been extended to also check for '!m->dvd'.  Once the first packet
>> has been checked, m->sofdec is either 1 or -1, so that code will
>> only ever be executed once.
>
> ok but then why is the check for dvd == 0 there ?
> its 0 on the first run and irrelevant after

Because if we're not dealing with a DVD, we should skip that packet 
completely, just like before my patch.  It's not irrelevant after.

On the first run, sofdec is zero and dvd is zero, so we enter the 
'checking code'.  That will set sofdec to either -1 (not sofdec) or 1 
(is sofdec), and if sofdec is not detected but a DVD packet is, dvd will 
be set to 1 so the possibilities after running the checking code on the 
first packet are:

sofdec = -1
dvd    = 0
        = Unidentified packet - skip

sofdec = 1
dvd    = 0
        = Sofdec packet - skip

sofdec = -1
dvd    = 1
        = DVD packet, don't skip

The checking code is only run once.

>>>> +            if (m->sofdec <= 0 &&
>>>> +                ((origlen == 980  && firstbyte == 0) ||
>>>> +                 (origlen == 1018 && firstbyte == 1))) {
>>>> +                /* DVD NAV packet, move back to the start of the stream (plus 'length' field) */
>>>> +                m->dvd = 1;
>>>> +                if (avio_skip(s->pb, -((origlen-len) + 2)) < 0) {
>>>
>>> if you do seek back anyway then you could check more than 1 byte of
>>> the packet
>>
>> To be honest, there's not much else that can be checked.  The PCI
>> packet (firstbyte == 0) has the two fields vobu_s_ptm and
>> vobu_e_ptm, which could be checked to ensure vobu_e_ptm > vobu_s_ptm
>> but that's about it.  The DSI packet (firstbyte == 1) doesn't really
>> have anything that can be easily used as an identifier.
>
> i see a BCD coded field, i see a table for 36 buttons with starting
> and ending x and y positions, i assume that that needs end >= start

vobu end pts >= vobu start pts is about the only thing.  The button 
table may be empty.  Not every NAV block defines buttons.

The cell elapsed time (if that's the BCD field you mean) can be almost 
anything.

> i see a SCR value, whichi would assume has to be similar to other
> SCR values

The checking code only runs once so there's not much to check it 
against.  The first SCR encountered could very conceivably be all 
zeroes, but even if not, surely any value is possible?

> i see several fields containing sector based addresses, forward and
> backward pointer tables (for seeking?)

Which may or may not be set, and if they are set, can be basically anything.

> so if i dont miss anything theres quite a bit more than 8bit that
> can be checked (and the 8bit == 0 check is super weak because 0 bytes
> are likely common)

It's not that there aren't any fields, just that they may or may not be 
populated.  And you have to be careful saying things like 'this field 
can only be 0, 1 or 2' because that field may only be read if another 
field is set to a certain value.  I've seen at least one DVD with 
'invalid' data in a field that didn't affect playback because another 
field had 'deactivated' it.  Looking at it from a strict viewpoint, it's 
invalid.  Playback had no issue because it never needed to read it.

I agree that the check of the first byte is not ideal (and to mitigate 
it somewhat, the length of the packet has to match too), but there's no 
long magic number to easily identify the packets.

Assuming there were a sufficient number of bits here and there that 
could be used, it would be mean re-writing the checking code to 
dynamically allocate a buffer (up to 65k) to read the whole packet in 
first and then run the checks on that buffer before freeing it up again. 
  Otherwise we'd be reading/seeking/reading/seeking.  I've not managed 
to get an answer as to whether that would be acceptable.

Richard.