[FFmpeg-devel] [PATCH] First shot at AVCHD seeking via new seeking API

Diego Biurrun diego
Wed Aug 26 11:12:51 CEST 2009


On Sun, Aug 16, 2009 at 11:47:02AM +0200, Ivan Schreter wrote:
> 
> I think it's clean and 100% exact now.
> 
> --- libavformat/seek.c	(revision 0)
> +++ libavformat/seek.c	(revision 0)
> @@ -0,0 +1,578 @@
> +/*
> + * Utility functions for seeking for use within FFmpeg format handlers.

seek utility functions for use within format handlers

> +// NOTE: implementation should be moved here in another patch, to keep patches
> +// separated.
> +extern void av_read_frame_flush(AVFormatContext *s);

I think the extern keyword is useless.  But this should really be in a
header file.

> +/**
> + * Helper structure to store parser state of AVStream.

This non-sentence lacks a verb, I suggest lowercasing and dropping the
period.  The same applies to all other (Doxygen) comments below.

> +    if (ts_a == INT64_MIN)
> +        return ts_a < ts_b ? -1 : 0;
> +    if (ts_a == INT64_MAX)
> +        return ts_a > ts_b ? 1 : 0;
> +    if (ts_b == INT64_MIN)
> +        return ts_a > ts_b ? 1 : 0;
> +    if (ts_b == INT64_MAX)
> +        return ts_a < ts_b ? -1 : 0;

nit: Align the 1s.

> +
> +    a = ts_a * tb_a.num * tb_b.den;
> +    b = ts_b * tb_b.num * tb_a.den;
> +
> +    res = a - b;
> +    if (res == 0)

if (!res)

> +static int64_t ts_distance(int64_t ts_hi, AVRational tb_hi, int64_t ts_lo, AVRational tb_lo)

long line

> + * Partial search for keyframes in multiple streams.
> + *
> + * This routine searches for the next lower and next higher timestamp to
> + * given target timestamp in each stream, starting at current file position
> + * and ending at position, where all streams have already been examined
> + * (or when all higher key frames found in first iteration).

This sounds awkward.  Maybe you could start by splitting it into 2-3
sentences, four lines is too long for a single sentence.

> + * This routine is called iteratively with exponential backoff to find lower
> + * timestamp.

the lower

> +        if (st->discard >= AVDISCARD_ALL) {
> +            // This stream is not active, skip packet.
> +            continue;
> +        }
> +
> +        if (pts == AV_NOPTS_VALUE) {
> +            // Some formats don't provide PTS, only DTS.
> +            pts = dts;
> +        }

pointless {}

> +    if (!sync) {
> +        // cannot allocate helper structure
> +        return -1;
> +    }

ditto

> +    if (keyframes_to_find == 0) {

if (!keyframes_to_find) {

> --- libavformat/seek.h	(revision 0)
> +++ libavformat/seek.h	(revision 0)
> @@ -0,0 +1,97 @@
> +/*
> + * Utility functions for seeking for use within FFmpeg format handlers.

see above

> +/// Opaque structure for parser state.
> +typedef struct AVParserState AVParserState;

I don't like such typedefs.  Others seem to disagree though.

> + * A sync point is a point in stream, so that decoding from this point,
> + * output of decoders of all streams synchronizes closest to given timestamp
> + * ts (but taking timestamp limits into account, i.e., no sooner than ts_min
> + * and no later than ts_max).

I'm not sure I understood this correctly, the length of the sentence is
not helping.

> +/**
> + * Store current parser state and file position.
> + *
> + * This function can be used by demuxers before destructive seeking algorithm

before a

> --- libavformat/mpegts.c	(revision 19507)
> +++ libavformat/mpegts.c	(working copy)
> @@ -1520,29 +1521,81 @@
>  
> -static int read_seek(AVFormatContext *s, int stream_index, int64_t target_ts, int flags){
> -    MpegTSContext *ts = s->priv_data;
> -    uint8_t buf[TS_PACKET_SIZE];
> +static int read_seek(AVFormatContext *s, int stream_index, int64_t target_ts, int flags);

ugly forward declaration

> +static int read_seek2(AVFormatContext *s, int stream_index, int64_t min_ts, int64_t target_ts, int64_t max_ts, int flags)

long line

> +        if(stream_index < 0){

if (stream_index < 0) {


Many of your lines could easily stay within the 80 character limit.
I haven't reviewed your documentation in much detail, but try to split
your sentences, they are generally long and intertwined.  Also, your
prose is in need of more articles.  You leave out "the" and "a" in many
cases where I think they are necessary and would help the readability of
the documentation.

Diego



More information about the ffmpeg-devel mailing list