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

Baptiste Coudurier baptiste.coudurier
Thu Aug 27 00:02:50 CEST 2009


On 08/26/2009 02:26 PM, Ivan Schreter wrote:
> 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 think this is useless if header is not public.
Using opaque struct can be usefull to prevent ABI breakage, forcing 
users to use API.

Please move the complete declaration in seek.h

>> [...]
>> 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.

There are absolutely no casts on malloc in the whole FFmpeg tree.
IIRC it does not produce a warning in C. Please remove the casts.

> Do you still have some other open points than already discussed last
> month?

Yes.

> Shall I disable the new seeking code in mpegts, or can we leave it for now as-is?

I'll test the code a bit more. Please address all comments ASAP and fix 
it in svn.

No need to disable it yet I think, however I'd like to point that 
committing this way given the state of the patch and the review is 
unacceptable.

[...]

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



More information about the ffmpeg-devel mailing list