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

[Richard]

On 03/03/13 01:55, Michael Niedermayer wrote:
> On Sun, Mar 03, 2013 at 12:51:46AM +0100, Richard wrote:
>> On 02/03/13 13:18, Michael Niedermayer wrote:
> [...]
>> +static int dvd_nav_parse(AVCodecParserContext *s,
>> +                         AVCodecContext *avctx,
>> +                         const uint8_t **poutbuf, int *poutbuf_size,
>> +                         const uint8_t *buf, int buf_size)
>> +{
>> +    DVDNavParseContext *pc1 = s->priv_data;
>> +    ParseContext *pc        = &pc1->pc;
>> +    int lastPacket          = 0;
>> +    int valid               = 0;
>> +
>> +    s->pict_type = AV_PICTURE_TYPE_NONE;
>> +
>
>> +    avctx->time_base.num = 1;
>> +    avctx->time_base.den = 90000;
>
> why is this needed ?

Because otherwise the AVPacket returned to the calling application ends 
up with a duration of 0.

<snip>

>> +        }
>> +    }
>> +
>> +    if (!valid || lastPacket) {
>> +        pc1->copied = 0;
>> +        pc1->lba    = 0xFFFFFFFF;
>> +    }
>
> the {} placement in this file is rather inconsistent

Yes, you're right.  Sorry, I was trying to keep to K&R style, which 
seems to be the main style used, but which isn't my normal style.  I'll 
tidy that up.

<snip>

>> diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c
>> index 4eaffd8..98ddc88 100644
>> --- a/libavformat/mpeg.c
>> +++ b/libavformat/mpeg.c
>> @@ -247,9 +247,12 @@ static int mpegps_read_pes_header(AVFormatContext *s,
>>           goto redo;
>>       }
>>       if (startcode == PRIVATE_STREAM_2) {
>> -        len = avio_rb16(s->pb);
>> +        int origlen = len = avio_rb16(s->pb);
>> +        uint8_t firstbyte = avio_r8(s->pb);
>> +        avio_skip(s->pb, -1);
>>           if (!m->sofdec) {
>> -            while (len-- >= 6) {
>> +            while (len >= 6) {
>> +                len--;
>>                   if (avio_r8(s->pb) == 'S') {
>>                       uint8_t buf[5];
>>                       avio_read(s->pb, buf, sizeof(buf));
>> @@ -260,8 +263,15 @@ static int mpegps_read_pes_header(AVFormatContext *s,
>>               }
>>               m->sofdec -= !m->sofdec;
>>           }
>> -        avio_skip(s->pb, len);
>> -        goto redo;
>> +        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) */
>
>> +            avio_skip(s->pb, -((origlen-len) + 2));
>
> this seeks backward which is not guranteed to work on non seekable
> protocols. On a dvd it would work but might if the OS cannot satisfy
> it from some cache do an actual seek which is slow.
> the same issue exists a fiw lines above

Yes, I wasn't completely happy with that myself.  The problem is that 
the contents need to be inspected to determine the codec for the packet 
(in the case of a DVD) or the codec to use for audio (in the case of 
Sofdec).  But inspecting the contents here prevents them being passed on 
without a negative seek.  How should/can that be solved here?

Richard.