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

[Baptiste Coudurier]

On 08/09/2009 07:50 AM, Ivan Schreter wrote:
> Hi Baptiste,
>
> Ivan Schreter wrote:
>> [...]
>> Ok, so I'll implement your proposal regarding active flag and the
>> optimization for ts == max_ts. Per-stream term position is in my
>> opinion still necessary. Will it be OK to commit then?
> Attached is the newest version of the patch.
>
> I've addressed these points:
>
> * Removed "active" flag and replaced it with st->discard checking.
> * Also removed "found_hi" and "found_lo" flags, as the position can
> be used for that.
> * Added optimization for ts < ts_max by initializing terminating
> condition to ts_max for first iteration. Also, instead of
> positions, timestamps are used per-stream as terminating condition
> (makes the code for TS and byte seeking exactly the same).
> * Removed "fpos" approximation of frame position and instead keeping
> last-known position for multi-frame packets.
> * Per your request, I added saving/restoring of parser state to
> seek.c and calls to it in mpegts.c, so seeking to invalid position
> won't invalidate current stream state (or in other words, it will
> restore the state just before seeking, so next frame will be read
> as if no seek was attempted). This can be used generically, so I
> exposed it in seek.h.
>
> [...]
>
> +
> +/**
> + * Helper structure describing keyframe search state of one stream.
> + */
> +typedef struct {
> +    int64_t pos_lo;     ///<  Position of the frame with low timestamp in file or INT64_MAX if not found (yet).
> +    int64_t pts_lo;     ///<  Frame presentation timestamp or same as pos_lo for byte seeking.
> +
> +    int64_t pos_hi;     ///<  Position of the frame with high timestamp in file or INT64_MAX if not found (yet).
> +    int64_t pts_hi;     ///<  Frame presentation timestamp or same as pos_hi for byte seeking.
> +
> +    int64_t last_pos;   ///<  Last known position of a frame, for multi-frame packets.

 > [...]
 >
> +    int64_t term_ts;    ///<  Termination timestamp (which TS we already read).
> +    int64_t first_ts;   ///<  First packet timestamp in this iteration (to fill term_ts later).
> +    int     terminated; ///<  Termination flag for current iteration.

I still don't like these fields, especially terminated.

 > [...]
 >
> +                    if (sp->pos_hi == INT64_MAX) {
> +                        // No high frame exists for this stream
> +                        (*found_hi)++;
> +                        sp->pts_hi = INT64_MAX;
> +                        sp->pos_hi = INT64_MAX - 1;

Why are you setting these fields to these values ?

> [...]
>
> +
> +            if (ts<= timestamp) {
> +                // Keyframe found before target timestamp.
> +                if (sp->pos_lo == INT64_MAX) {
> +                    // Found first keyframe lower than target timestamp.
> +                    (*found_lo)++;
> +                    sp->pts_lo = ts;
> +                    sp->pos_lo = pos;
> +                } else if (sp->pts_lo<  ts) {
> +                    // Found a better match (closer to target timestamp).
> +                    sp->pts_lo = ts;
> +                    sp->pos_lo = pos;
> +                }

Hummm, it's the same code, if (sp->pos_lo == INT64_MAX || sp->pts_lo < ts) ?

It will simplify the code.

Btw why don't you set pos_lo to INT64_MIN ? It would make more sense.

> +            }
> +            if (ts>= timestamp) {
> +                // Keyframe found after target timestamp.
> +                if (sp->pos_hi == INT64_MAX) {
> +                    // Found first keyframe higher than target timestamp.
> +                    (*found_hi)++;
> +                    sp->pts_hi = ts;
> +                    sp->pos_hi = pos;
> +                    if (*found_hi>= keyframes_to_find&&  first_iter) {
> +                        // We found high frame for all. They may get updated
> +                        // to TS closer to target TS in later iterations (which
> +                        // will stop at start position of previous iteration).
> +                        break;
> +                    }
> +                } else if (sp->pts_hi>  ts) {
> +                    // Found a better match (actually, shouldn't happen).
> +                    sp->pts_hi = ts;
> +                    sp->pos_hi = pos;
> +                }

Same comment.

 > [...]
>
> +}
> +
> +static void free_packet_list(AVPacketList *pktl)
> +{
> +    AVPacketList *cur;
> +    while (pktl) {
> +        cur = pktl;
> +        pktl = cur->next;
> +        av_free_packet(&cur->pkt);
> +        av_free(cur);
> +    }

I think this function would be useful in utils.c and flush_packet_queue 
could use it.

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org