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

[Diego Biurrun]

Please leave empty lines between your text and the text you quote,
otherwise your mails are unnecessarily hard to read, thanks.

On Wed, Aug 26, 2009 at 10:38:03PM +0200, Ivan Schreter wrote:
> 
> 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 @@
> >>+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?

If it gets fixed in the near future, I don't mind.

> >>+/**
> >>+ * 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.

I reread the above paragraph three times.  I have no idea what you are
referring.  Apparently you must have read a lot into my words, but I can
assure you I meant nothing except what I wrote.  Let me try again.

This non-sentence lacks a verb.  I suggest you lowercase the first word
and drop the period at the end of it.  The same applies to all other
(Doxygen) comments below.

> >>--- libavformat/seek.h	(revision 0)
> >>+++ libavformat/seek.h	(revision 0)
> >>@@ -0,0 +1,97 @@
> >>+/// 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?

Yes, writing 'struct' is not harmful to anybody's health :)

> >>--- libavformat/mpegts.c	(revision 19507)
> >>+++ libavformat/mpegts.c	(working copy)
> >>@@ -1520,29 +1521,81 @@
> 
> >
> >Many of your lines could easily stay within the 80 character limit.
> >  
> Actually, I usually take care to fit into 80 characters limit.

Hmm...

> 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 hear your words, but I see your patch (needlessly) push many lines
past the 80 character mark..

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

It's much better.

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

Just commit it taking my other comments into account.  It's harmless but
it improves the quality of the patch Baptiste and Michael will review.

> --- libavformat/seek.c	(revision 19681)
> +++ libavformat/seek.c	(working copy)
> @@ -1,5 +1,5 @@
>  /*
> - * Utility functions for seeking for use within FFmpeg format handlers.
> + * Seek utility functions for use within format handlers.

This is not a sentence.  Lowercase it and drop the period.

> @@ -63,30 +63,40 @@
>   * Helper structure describing keyframe search state of one stream.
>   */
>  typedef struct {
> -    int64_t pos_lo;     ///< Position of the frame with low timestamp in file or INT64_MAX if not found (yet).
> -    int64_t ts_lo;      ///< Frame presentation timestamp or same as pos_lo for byte seeking.
> +    /// Position of the frame with low timestamp in file or INT64_MAX if not found (yet).
> +    int64_t     pos_lo;
> +    /// Frame presentation timestamp or same as pos_lo for byte seeking.
> +    int64_t     ts_lo;

Michael likes such Doxygen comments on the right side.

>  /**
> - * Compare two timestamps exactly, taking into account their respective time bases.
> + * Compare two timestamps exactly, taking their respective time bases into account.

This is better.

> - * @return -1. 0 or 1 if timestamp A is less than, equal or greater than timestamp B.
> + * @return -1, 0 or 1 if timestamp A is less than, equal or greater than timestamp B.

equal to

> @@ -216,14 +229,15 @@
>  
>          // Evaluate key frames with known TS (or any frames, if AVSEEK_FLAG_ANY set).
> -        if (pts != AV_NOPTS_VALUE && ((flg & PKT_FLAG_KEY) || (flags & AVSEEK_FLAG_ANY))) {
> +        if (pts != AV_NOPTS_VALUE && 
> +            ((flg & PKT_FLAG_KEY) || (flags & AVSEEK_FLAG_ANY)))
> +        {

This is not K&R, same below.

> @@ -459,7 +477,8 @@
>  
> -    state->stream_states = (AVStreamState*) av_malloc(sizeof(AVStreamState) * s->nb_streams);
> +    state->stream_states = (AVStreamState*) av_malloc(
> +                           sizeof(AVStreamState) * s->nb_streams);

This is ugly.

> --- libavformat/seek.h	(revision 19681)
> +++ libavformat/seek.h	(working copy)
> @@ -29,15 +29,15 @@
>  
>  /**
> - * Search for sync point of all active streams.
> + * Search for the sync point of all active streams.
>   *
> - * This is not supposed to be called directly by a user application,
> + * This routine is not supposed to be called directly by a user application,
>   * but by demuxers.
>   *
> - * 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).
> + * A sync point is defined as a point in stream, such as when decoding from this point,
> + * output of decoders of all streams synchronizes closest to the given timestamp
> + * ts. This routine also takes timestamp limits into account. Thus, the output
> + * will synchronize no sooner than ts_min and no later than ts_max.

Let me try:

 * A sync point is defined as a point in a stream, such that, when decoding
 * starts from this point, the decoded output of all streams synchronizes
 * closest to the given timestamp ts.

This could still be improved though.

> @@ -61,13 +61,13 @@
>  /**
>   * Store current parser state and file position.
>   *
> - * This function can be used by demuxers before destructive seeking algorithm
> + * This call can be used by demuxers before a destructive seeking algorithm

"Call" does not sound better than function to me..

Diego