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

[Ivan Schreter]

Hi Baptiste,

Baptiste Coudurier wrote:
> [...]
>>
>>>> +// 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.
>> That's true. I would actually prefer to have the implementation in
>> seek.c and declaration in seek.h, since it's a seeking-relevant routine.
>> But I didn't want to do too much in one patch (is big enough as it is).
>> Therefore I'd prefer to leave it as is for now. After the thing is
>> clear, I'll clean it up by a trivial patch moving the routine to
>> seek.[ch], OK?
>
> Probably all seek related functions should be moved to seek.c
> as well as av_read_frame_flush, it might only be used in seek.c and 
> therefore be back static.
That's my intention for the long term - move seek-related support 
functions from utils.c to seek.c.

>
>> [...]
>>
>>>> +/// Opaque structure for parser state.
>>>> +typedef struct AVParserState AVParserState;
>>>
>>> I don't like such typedefs. Others seem to disagree though.
>> Well... If it's just your personal liking, then I'll leave it as is...
>>
>> But out of curiosity, do you just use "struct XXX" instead of "typedef
>> struct XXX XXX" or do you do it in a different way?
>
> I prefer:
>
> typedef struct {
>  ...
> } Type;
>
> In once, remove the useless opaque declaration in seek.h
> seek.h is not a public header.
True, one can do it that way.

OTOH, I learned my lessons regarding separation of concerns. Since the 
structure is completely internal to seek.c, why should it be visible 
even to the format implementer guy who is using seek.h routines? He 
can't care less... But it does eventually create unneeded include 
dependencies (I didn't check, though).

What do you think about that?

> [...]
> I'm testing it. I experience weird problems when seek fails currently.
What kind of problems? Seek can basically only fail if there is no 
appropriate key frame in min_ts to max_ts range. Otherwise it should 
always reposition the stream. Or do you mean the state save/restore on 
seek failure doesn't work as expected and further decoding fails?

> I'll review.
>
> > [...]
>>
>>       // Initialize syncpoint structures for each stream.
>>       sync = (AVSyncPoint*) av_malloc(s->nb_streams * 
>> sizeof(AVSyncPoint));
>
> Useless casts.
Mhm, true, at least gcc on Linux doesn't complain about type safety.

I'm used to multi-platform development with 10 different Unix platforms, 
so I write portable warning-free code. And conversion from void* to 
pointer to some type often produces a warning (IMHO it's pretty 
reasonable). But I can remove those casts.

Do you still have some other open points than already discussed last 
month? Shall I disable the new seeking code in mpegts, or can we leave 
it for now as-is?

Thanks & regards,

Ivan