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

[Ivan Schreter]

Hi Baptiste,

sorry for getting back to you so late, but unfortunately, I don't have 
much time to work on FFmpeg :-(

Baptiste Coudurier wrote:
> [...]
>> +    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.
I know you don't like them, but IMHO that's the only way to handle 
interleaved streams like MPEG-TS properly.

An alternative would be keeping one position or timestamp which contains 
local maximum of position or timestamp of first packets from all streams 
from previous iteration (oh, complicated to understand :-) and in 
current iteration reading until all streams return a packet with a 
position past this position or timestamp.

The disadvantage with a single position/timestamp is, you have to 
evaluate packets of one stream several times then. It is no harm, but 
with current code, I feel it's cleaner.

Simply reading until fpos is >= previous start fpos is not enough, since 
there are interleaved packets, which actually cross the position, as I 
already pointed out twice.

>
> > [...]
> >
>> +                    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 ?
>
As the comment says, we didn't find a high frame for the stream. PTS is 
thus set to invalid. Position must be set to something else, otherwise 
it could run into this or another if block comparing undefined pos_hi 
(effectively, meaning high frame not yet found) and then increment 
number of found high frames again. But number of found high frames must 
be incremented exactly once per stream.

Alternative would be to introduce new fields found_hi and found_lo in 
syncpoint structure per stream and instead of testing pos_hi/pos_lo for 
INT64_MAX, check those flags. Shall I rework it that way?

>> [...]
>>
>> +
>> +            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.
>
It's not the same code. In first branch, number of already-found low 
frames is increased, in other one not. Thus, if I'd do it in one block, 
I'd have to add another if condition inside the block checking pos_lo 
for INT64_MAX and only then incrementing number of found lo frames.

But if you wish, I can rewrite it that way. Shall I do it?

> Btw why don't you set pos_lo to INT64_MIN ? It would make more sense.
>
Both would do. It's basically a flag value. Would you set pos_lo to 
INT64_MIN? What then with pos_hi? Maybe AV_NOPTS_VALUE would be more proper?

What do you think?


>> +            }
>> +            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.
Again, same as above. In the if() condition, another nested if() would 
be needed.

>
> > [...]
>>
>> +}
>> +
>> +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.
>
Surely. Let's keep this on a TODO list. Is there somewhere a common one?

I think it generally makes sense to reorganize the code a bit 
(especially seeking-related functions) and put some functions elsewhere.

BTW, I found out that pure seeking routine time can be reduced about 
factor 3 for full-HD AVCHD files, when using higher initial step size 
(32KB) for back-off. I will investigate how this step size could be 
automagically determined optimally.

Regards,

Ivan