[FFmpeg-devel] [PATCH 1/2] add support for generic seeking by?reading timestamps in nuv

[Joakim Plate]

On Mon, 12 Sep 2011 23:21:36 +0200, Reimar Döffinger wrote:

> On Mon, Sep 12, 2011 at 07:51:08PM +0200, Joakim Plate wrote:
>> + * \return TRUE if the syncword is found.
> 
> 1, not TRUE

Fixed

>> +static int64_t nuv_read_dts(AVFormatContext *s, int stream_index,
>> +                               int64_t *ppos, int64_t pos_limit)
> 
> Strange indentation.

Fixed

> 
>> +        if (avio_read(pb, hdr, HDRSIZE) <= 0)
> 
> Shouldn't that check for < HDRSIZE?

Fixed

> 
>> +                av_add_index_entry(s->streams[frametype == NUV_VIDEO ? ctx->v_id : ctx->a_id]
>> +                                 , pos, dts, size + HDRSIZE, 0, hdr[2] == 0 ? AVINDEX_KEYFRAME : 0);
> 
> , is strangely placed.

Fixed

> Also since you do not check that v_id/a_id is >= 0 that can crash in a
> file that is marked to contain no stream for that packet type.
> Also IIRC your other patch uses hdr[2] only for video, this uses it for audio
> and video, one isn't right...

Fixed

> 
>> +                if ((frametype == NUV_VIDEO && stream_index == ctx->v_id) ||
>> +                    (frametype == NUV_AUDIO && stream_index == ctx->a_id)) {
> 
> Also this duplicates code, I think you'd want a
> stream_id = frametype == NUV_VIDEO ? ctx->v_id : ctx->a_id;
> if (stream_id < 0) ...
Fixed


I'm a bit tired right now. Really should have gone to bed already. But what
about this?

/Joakim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-support-for-generic-seeking-by-reading-timestamp.patch
Type: text/x-patch
Size: 3430 bytes
Desc: Attached file: 0001-add-support-for-generic-seeking-by-reading-timestamp.patch
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110913/9f9e4626/attachment.bin>