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

[Baptiste Coudurier]

On 8/26/2009 1:38 PM, Ivan Schreter wrote:
> Hi Diego,
>
> Diego Biurrun wrote:
>> On Wed, Aug 26, 2009 at 08:52:40AM +0200, Ivan Schreter wrote:
>>> Michael Niedermayer wrote:
>>>> On Tue, Aug 25, 2009 at 02:25:57PM -0700, Baptiste Coudurier wrote:
>>>>> Please revert the patch for now.
>>>> i didnt want to say it without reviewing the actually commited code,
>>>> (for which i didnt yet had time) but i too felt this should be reverted
>>>> and reviewed first.
>>>>
>>> Uhm...
>>> [...]
>>
>> Not that I mind reverting too much (that's what revision control is for
>> after all), but maybe we can have the review first? At a glance it
>> looks to me as though this can be resolved without reverting.
> First, thanks for your support :-).
> As I wrote, I believed the review is finished and I had no bad
> intentions with submitting the patch.
>
> Diego Biurrun wrote:
>> 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
> OK.
>>> +// 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.

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

 >> [...]
>>
>> Many of your lines could easily stay within the 80 character limit.
> Actually, I usually take care to fit into 80 characters limit. But there
> are a few lines, where it didn't make much sense to split. But I've
> split them now and also re-done some doxygen comments, so they don't
> exceed 80 characters.
>
>> 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.
> Hm, I'm not a native speaker so I do tend to leave out some articles.
> But I'm afraid, this can't be helped that easy :-). I've tried to add at
> least some missing articles. I hope I didn't overdo it...
>
> I've attached the cosmetics patch according to your comments above (plus
> some additional cosmetics as mentioned above). If it's OK from your
> side, feel free to commit it.
>
> As for the functional review, as I wrote, I felt the review is actually
> finished. So should I disable the new code in mpegts for now or not?
> Neither Baptiste nor Michael answered this question yet...

I'm testing it. I experience weird problems when seek fails currently.
I'll review.

 > [...]
>
>       // Initialize syncpoint structures for each stream.
>       sync = (AVSyncPoint*) av_malloc(s->nb_streams * sizeof(AVSyncPoint));

Useless casts.

[...]

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