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

Ivan Schreter schreter
Wed Aug 26 22:38:03 CEST 2009


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?

>   
>> +/**
>> + * Helper structure to store parser state of AVStream.
>>     
>
> This non-sentence lacks a verb, I suggest lowercasing and dropping the
> period.  The same applies to all other (Doxygen) comments below.
>
>   
It is actually intentionally a non-sentence. It's explanation of what is 
in the structure. How would you describe it?

By starting "This is blabla" the documentation isn't clearer, rather 
harder to read. I think a non-sentence description like the one here is 
just right for members/structures, except where you need to put some 
more prosa in it.

>> +    if (ts_a == INT64_MIN)
>> +        return ts_a < ts_b ? -1 : 0;
>> +    if (ts_a == INT64_MAX)
>> +        return ts_a > ts_b ? 1 : 0;
>> +    if (ts_b == INT64_MIN)
>> +        return ts_a > ts_b ? 1 : 0;
>> +    if (ts_b == INT64_MAX)
>> +        return ts_a < ts_b ? -1 : 0;
>>     
>
> nit: Align the 1s.
>   
OK.

>   
>> +
>> +    a = ts_a * tb_a.num * tb_b.den;
>> +    b = ts_b * tb_b.num * tb_a.den;
>> +
>> +    res = a - b;
>> +    if (res == 0)
>>     
>
> if (!res)
>   
OK.

>   
>> +static int64_t ts_distance(int64_t ts_hi, AVRational tb_hi, int64_t ts_lo, AVRational tb_lo)
>>     
>
> long line
>   
OK, put the parameters one per line.

>   
>> + * Partial search for keyframes in multiple streams.
>> + *
>> + * This routine searches for the next lower and next higher timestamp to
>> + * given target timestamp in each stream, starting at current file position
>> + * and ending at position, where all streams have already been examined
>> + * (or when all higher key frames found in first iteration).
>>     
>
> This sounds awkward.  Maybe you could start by splitting it into 2-3
> sentences, four lines is too long for a single sentence.
>   
OK, please check if it's clearer now in the attached patch.

>   
>> + * This routine is called iteratively with exponential backoff to find lower
>> + * timestamp.
>>     
>
> the lower
>   
OK.

>   
>> +        if (st->discard >= AVDISCARD_ALL) {
>> +            // This stream is not active, skip packet.
>> +            continue;
>> +        }
>> +
>> +        if (pts == AV_NOPTS_VALUE) {
>> +            // Some formats don't provide PTS, only DTS.
>> +            pts = dts;
>> +        }
>>     
>
> pointless {}
>   
OK. I removed it and the others as well (were some more than you spotted).

>   
>> +    if (!sync) {
>> +        // cannot allocate helper structure
>> +        return -1;
>> +    }
>>     
>
> ditto
>
>   
>> +    if (keyframes_to_find == 0) {
>>     
>
> if (!keyframes_to_find) {
>   
OK.

>   
>> --- libavformat/seek.h	(revision 0)
>> +++ libavformat/seek.h	(revision 0)
>> @@ -0,0 +1,97 @@
>> +/*
>> + * Utility functions for seeking for use within FFmpeg format handlers.
>>     
>
> see above
>   
Changed as above.

>   
>> +/// 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?

>   
>> + * A sync point is a point in stream, so that decoding from this point,
>> + * output of decoders of all streams synchronizes closest to given timestamp
>> + * ts (but taking timestamp limits into account, i.e., no sooner than ts_min
>> + * and no later than ts_max).
>>     
>
> I'm not sure I understood this correctly, the length of the sentence is
> not helping.
>   
I split it a bit.

>   
>> +/**
>> + * Store current parser state and file position.
>> + *
>> + * This function can be used by demuxers before destructive seeking algorithm
>>     
>
> before a
>   
OK.

>   
>> --- libavformat/mpegts.c	(revision 19507)
>> +++ libavformat/mpegts.c	(working copy)
>> @@ -1520,29 +1521,81 @@
>>  
>> -static int read_seek(AVFormatContext *s, int stream_index, int64_t target_ts, int flags){
>> -    MpegTSContext *ts = s->priv_data;
>> -    uint8_t buf[TS_PACKET_SIZE];
>> +static int read_seek(AVFormatContext *s, int stream_index, int64_t target_ts, int flags);
>>     
>
> ugly forward declaration
>   
True. Ugly and not needed anymore (was just for testing in itermediate 
step). Removed.

>   
>> +static int read_seek2(AVFormatContext *s, int stream_index, int64_t min_ts, int64_t target_ts, int64_t max_ts, int flags)
>>     
>
> long line
>   
Split.

>   
>> +        if(stream_index < 0){
>>     
>
> if (stream_index < 0) {
>   
OK.

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

BTW, the only known issue (not a bug) with the new seeking code is 
seeking in H.264 files without keyframes (well, in those files, where 
keyframes are not recognized properly, because they are lacking SEI 
buffering period). But this is a separate issue addressed in another 
thread some time ago and a solution has also been discussed (but not 
done yet, I plan to implement it, though).

Thanks & regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: seek_cosmetics.patch
Type: text/x-patch
Size: 16967 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090826/fc600c41/attachment.bin>



More information about the ffmpeg-devel mailing list