[FFmpeg-devel] [RFC] Seeking in PVA files

[Michael Niedermayer]

On Sat, Jan 05, 2008 at 08:23:25PM +0100, Ivo wrote:
> On Saturday 05 January 2008 18:44, Michael Niedermayer wrote:
> > av_seek_frame_binary() uses the index
> > av_index_search_timestamp() is not correct unless you have indexed the
> > whole file or at least a single continous area
> >
> > if you read the first 10% then seek to the end end agaib read 10% then
> > av_index_search_timestamp() will not fail if you seek between these 2 but
> > it will neither give you a reasonable result
> 
> I see. Basically read_seek if mostly for containers that have their own 
> index. I RTFS'd a bit more, which resulted in the attached patch. It also 
> includes the refactoring of mpeg.c so the pes ts code can be reused.
> 

[...]

>      st->codec->codec_type = CODEC_TYPE_AUDIO;
> -    st->codec->codec_id   = CODEC_ID_MP3;
> +    st->codec->codec_id   = CODEC_ID_MP2;
>      st->need_parsing      = AVSTREAM_PARSE_HEADERS;

seperate patch/commit!


[...]
> +static inline void read_pva_header(ByteIOContext *pb, int *syncword,
> +                    int *streamid, int *reserved, int *flags, int *length) {
> +    *syncword = get_be16(pb);
> +    *streamid = get_byte(pb);
> +    get_byte(pb);               /* counter not used */
> +    *reserved = get_byte(pb);
> +    *flags    = get_byte(pb);
> +    *length   = get_be16(pb);
> +}
> +
> +static inline void get_pes_header(ByteIOContext *pb, int *pes_signal,
> +        int *pes_packet_length, int *pes_flags, int *pes_header_data_length,
> +        int8_t *pes_header_data) {
> +    *pes_signal             = get_be24(pb);
> +    get_byte(pb);
> +    *pes_packet_length      = get_be16(pb);
> +    *pes_flags              = get_be16(pb);
> +    *pes_header_data_length = get_byte(pb);
> +    get_buffer(pb, pes_header_data, *pes_header_data_length);
> +}
> +
>  static int pva_read_packet(AVFormatContext *s, AVPacket *pkt) {
>      ByteIOContext *pb = s->pb;
>      PVAContext *pvactx = s->priv_data;
>      int ret, syncword, streamid, reserved, flags, length, pts_flag;
> -    long long pva_pts = 0;
> +    int64_t pva_pts = AV_NOPTS_VALUE, startpos = url_ftell(pb);
>  
> -    syncword = get_be16(pb);
> -    streamid = get_byte(pb);
> -    get_byte(pb);               /* counter not used */
> -    reserved = get_byte(pb);
> -    flags    = get_byte(pb);
> -    length   = get_be16(pb);

seperate patch/commit!
and there is no check for streamid being valid 


[...]
> -            unsigned char pes_header_data[256];
> +            uint8_t pes_header_data[256];

cosmetic -> ok but seperate commit please


[...]
> +static int64_t pva_read_timestamp(struct AVFormatContext *s, int stream_index,
> +                                  int64_t *pos, int64_t pos_limit) {
> +    ByteIOContext *pb = s->pb;
> +    PVAContext *pvactx = s->priv_data;
> +    int64_t res = AV_NOPTS_VALUE;
> +

> +    if (pos_limit - *pos > PVA_MAX_PAYLOAD_LENGTH * 16)
> +        pos_limit = *pos + PVA_MAX_PAYLOAD_LENGTH * 16;

FFMIN()


> +
> +    while (*pos < pos_limit) {
> +        int syncword, streamid, reserved, flags, length;
> +
> +        if (url_fseek(pb, *pos, SEEK_SET) < 0) return AV_NOPTS_VALUE;
> +

> +        read_pva_header(pb, &syncword, &streamid, &reserved, &flags, &length);
> +
> +        if (syncword != PVA_MAGIC || reserved != 0x55 || !streamid ||
> +            streamid >PVA_AUDIO_PAYLOAD || length > PVA_MAX_PAYLOAD_LENGTH) {
> +            (*pos)++;
> +            continue;
> +        }

shouldnt these checks be in read_pva_header() ? 


> +
> +        if (streamid - 1 == stream_index) {
> +            if (streamid == PVA_VIDEO_PAYLOAD && flags & 0x10) {
> +                res = get_be32(pb);
> +                break;
> +            } else if (streamid == PVA_AUDIO_PAYLOAD) {
> +                int pes_signal, pes_header_data_length, pes_packet_length,
> +                    pes_flags;
> +                uint8_t pes_header_data[256];
> +
> +                get_pes_header(pb, &pes_signal, &pes_packet_length, &pes_flags,
> +                               &pes_header_data_length, pes_header_data);
> +
> +                if (pes_signal == 1) {
> +                    if (pes_flags & 0x80 && (pes_header_data[0] & 0xf0)== 0x20)
> +                        res = ff_parse_pes_pts(pes_header_data);
> +                    if (res != AV_NOPTS_VALUE) break;
> +                }
> +            }
> +        }

you should parse all streams and use av_add_index_entry() even if the
specific stream hasnt been requested

also cant more code be factored with pva_read_packet() ?
this looks somewhat similar
if so you might even avoid the need for some of the new functions

[...]


> Index: libavformat/mpeg.c

needs mans approval

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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080105/87b4b9cf/attachment.pgp>