[FFmpeg-devel] [RFC/PATCH] Pass PRIVATE_STREAM_2 MPEG-PS packets to caller
Richard
peper03 at yahoo.com
Fri Mar 8 11:26:52 CET 2013
On 08/03/13 02:00, Michael Niedermayer wrote:
> On Fri, Mar 08, 2013 at 01:18:21AM +0100, Richard wrote:
>> On 03/03/13 16:09, Richard wrote:
>>> On 03/03/13 10:25, Richard wrote:
>>>> 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?
>>>
>>> Ok, attached is a patch that addresses the formatting/code style issues.
>>
>> And another that hopefully successfully addresses the negative seek
>> issue. Once a DVD stream has been determined, no more packet
>> inspection is performed, so any seeks will only happen once, when
>> the first PRIVATE_STREAM_2 packet is detected.
>>
>> Richard.
>>
>
>> libavcodec/Makefile | 1
>> libavcodec/allcodecs.c | 1
>> libavcodec/avcodec.h | 2
>> libavcodec/codec_desc.c | 6 ++
>> libavcodec/dvd_nav_parser.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>> libavcodec/version.h | 2
>> libavformat/mpeg.c | 40 ++++++++++++--
>> 7 files changed, 163 insertions(+), 7 deletions(-)
>> ebaf3ac5186dbc6c20539a9d4100b03b7976b2c5 0001-Add-passing-DVD-navigation-packets-startcode-0x1bf-t.patch
>> From cea0c4411f11979bee6289ea5abbcd6d2e7e692c Mon Sep 17 00:00:00 2001
>> From: Richard <peper03 at yahoo.com>
>> Date: Fri, 8 Mar 2013 01:07:59 +0100
>> Subject: [PATCH] Add passing DVD navigation packets (startcode 0x1bf) to
>> caller to allow better playback handling of DVDs. The two
>> types of packets (PCI and DSI) are passed untouched but
>> combined by the new codec ID AV_CODEC_ID_DVD_NAV. The
>> first 980 bytes in the packet contain the PCI data. The
>> next 1018 are the DSI data.
>>
>> ---
>> libavcodec/Makefile | 1 +
>> libavcodec/allcodecs.c | 1 +
>> libavcodec/avcodec.h | 2 +
>> libavcodec/codec_desc.c | 6 +++
>> libavcodec/dvd_nav_parser.c | 118 +++++++++++++++++++++++++++++++++++++++++++
>> libavcodec/version.h | 2 +-
>> libavformat/mpeg.c | 40 ++++++++++++---
>> 7 files changed, 163 insertions(+), 7 deletions(-)
>> create mode 100644 libavcodec/dvd_nav_parser.c
>
> [...]
>
>> 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..c890b0c 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,13 @@ 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);
>> + 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.
Alternatively, memory could be allocated to hold the entire packet but
is dynamically allocating and freeing memory a cleaner solution?
>> uint8_t buf[5];
>> avio_read(s->pb, buf, sizeof(buf));
>> @@ -259,9 +264,24 @@ static int mpegps_read_pes_header(AVFormatContext *s,
>> }
>> }
>> m->sofdec -= !m->sofdec;
>> +
>> + 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 needs a failure check and the failure must be handled
Is any negative return value a failure? Can you assume that no skip has
been performed in case of an error? If so, I would skip forward the
remaining bytes. That packet would be lost, but there's not much you
can do about that.
If you can't assume that the pointer hasn't moved, what do you do then?
Richard.
More information about the ffmpeg-devel
mailing list