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

[Michael Niedermayer]

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:
> >>      if (startcode == PRIVATE_STREAM_2) {
> >>-        len = avio_rb16(s->pb);
> >>-        if (!m->sofdec) {
> >>-            while (len-- >= 6) {
> >>+        if (!m->sofdec && !m->dvd) {
> >>+            int origlen = len = avio_rb16(s->pb);
> >>+            uint8_t firstbyte = avio_r8(s->pb);
> >>+            /* 'Unread' the first byte so we can search the whole
> >>+             * buffer for 'Sofdec'.  Skipping back 1 byte should
> >>+             * never incur a physical read as even with a 1 byte buffer
> >>+             * the 2nd byte wouldn't be physically read until requested.
> >>+             */
> >
> >>+            avio_skip(s->pb, -1);
> >
> >an av_assert0() is needed, here, i dont want to have to debug such
> >failures
> 
> Ok, added.
> 
> >>+
> >>+            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


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

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

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

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)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
-------------- 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/20130315/36a62d85/attachment.asc>