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

[Michael Niedermayer]

On Fri, Mar 08, 2013 at 09:41:50PM +0100, Richard wrote:
> On 08/03/13 11:26, Richard wrote:
> >On 08/03/13 02:00, Michael Niedermayer wrote:
> >>On Fri, Mar 08, 2013 at 01:18:21AM +0100, Richard wrote:
> >>>+            uint8_t firstbyte = avio_r8(s->pb);
> >>>+            avio_skip(s->pb, -1);
> >>>+
> >>>+            while (len >= 6) {
> >>>+                len--;
> >>>                  if (avio_r8(s->pb) == 'S') {
> >>
> >>this reads a byte then seeks back and reads it again
> >>this sure can be done without the seek
> >
> >Well, it can but it then ends up looking pretty ugly:
> >
> >uint8_t firstbyte = avio_r8(s->pb);
> >len--;
> >
> >while (len >= 6) {
> >     if (firstbyte == 'S' || avio_r8(s->pb) == 'S') {
> >         if (firstbyte != 'S')
> >             len--;
> >
> >         uint8_t buf[5];
> >         avio_read(s->pb, buf, sizeof(buf));
> >         m->sofdec = !memcmp(buf, "ofdec", 5);
> >         len -= sizeof(buf);
> >         break;
> >     }
> >     len--;
> >}
> >
> >Since (to my understanding) the avio buffer must have a size of at least
> >1, a seek of -1 should never be a problem.
> 
> <snip>
> 
> >>>+                avio_skip(s->pb, -((origlen-len) + 2));
> >>
> >>this needs a failure check and the failure must be handled
> 
> Ok, I've checked for failure at this point.  I've left the 'skip -1'
> as that should never cause a physical read.
[...]
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 67fdc25..1e08f09 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,7 +29,7 @@
>  #include "libavutil/avutil.h"
>  
>  #define LIBAVCODEC_VERSION_MAJOR 54
> -#define LIBAVCODEC_VERSION_MINOR 92
> +#define LIBAVCODEC_VERSION_MINOR 93
>  #define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c
> index 4eaffd8..c7afa14 100644
> --- a/libavformat/mpeg.c
> +++ b/libavformat/mpeg.c
> @@ -108,6 +108,7 @@ typedef struct MpegDemuxContext {
>      int32_t header_state;
>      unsigned char psm_es_type[256];
>      int sofdec;
> +    int dvd;
>  #if CONFIG_VOBSUB_DEMUXER
>      AVFormatContext *sub_ctx;
>      FFDemuxSubtitlesQueue q;
> @@ -247,9 +248,18 @@ static int mpegps_read_pes_header(AVFormatContext *s,
>          goto redo;
>      }
>      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


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



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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/2b4e98af/attachment.asc>