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

[Michael Niedermayer]

On Sat, Mar 16, 2013 at 12:08:16AM +0100, Richard wrote:
> 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 code in your patch is this:
if (!m->sofdec && !m->dvd) {

it results in
0, 0 and 0 for these 3 cases and 1 for the initial state

if (!m->sofdec)

does the same

if you dont like just !m->sofdec, then feel free to change it to
something like if(first_packet)



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

so you also have to check if there are buttons.
What am i missing here ?
A. looks like a nav packet without buttons
B. looks like a nav packet with valid buttons
C. not a nav packet


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

so its not a hh:mm:ss:ff field as the text states ?


> 
> >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?

if its before any other packet with a timestamp yes


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

i dont see a problem with reading that packet in if that is needed

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130316/6080aa55/attachment.asc>