[FFmpeg-devel] [PATCH] avformat/flvenc: add build_keyframes_index option

[Lou Logan]

On Thu, Nov 3, 2016, at 05:05 AM, Steven Liu wrote:
> Build keyframes index information into metadata.

Can be simplified to:

"Add keyframe index metadata."

> Be used to http VOD flv.

The above phrase is not very descriptive. I suggest something like:

"Used to facilitate seeking; particularly for HTTP pseudo streaming."

> Signed-off-by: Steven Liu <liuqi at gosun.com>
> ---
>  doc/muxers.texi      |    3 +
>  libavformat/flvenc.c |  323
>  ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 317 insertions(+), 9 deletions(-)
> 
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index 488ed43..1df7d97 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -147,6 +147,9 @@ Place AAC sequence header based on audio stream data.
>  
>  @item no_sequence_end
>  Disable sequence end tag.
> +
> + at item build_keyframes_index

"add_keyframe_index" makes more sense to me.

> +Build keyframes index infomations into flv metadata. Be used to http
> VOD..

You can reuse the commit message sentences here.

>  @end table
>  @end table
>  
> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> index e50f8e4..1f1b608 100644
> --- a/libavformat/flvenc.c
> +++ b/libavformat/flvenc.c
> @@ -24,6 +24,8 @@
>  #include "libavutil/intfloat.h"
>  #include "libavutil/avassert.h"
>  #include "libavutil/mathematics.h"
> +#include "avio_internal.h"
> +#include "avio.h"
>  #include "avc.h"
>  #include "avformat.h"
>  #include "flv.h"
> @@ -64,8 +66,15 @@ static const AVCodecTag flv_audio_codec_ids[] = {
>  typedef enum {
>      FLV_AAC_SEQ_HEADER_DETECT = (1 << 0),
>      FLV_NO_SEQUENCE_END = (1 << 1),
> +    FLV_BUILD_KEYFRAME_IDX = (1 << 2),

I would name it FLV_ADD_KEYFRAME_INDEX or similar, but I'll leave it to
whatever you prefer.

[...]
> +    if (ret < 0) {
> +        av_log(s, AV_LOG_ERROR, "Unable to re-open %s output file for "
> +               "the second pass (faststart)\n", s->filename);
> +        goto end;
> +    }

"faststart" implies that an option or value named "faststart" was used.
Should be renamed.

> +    /* mark the end of the shift to up to the last data we wrote, and
> get ready
> +     * for writing */
> +    pos_end = avio_tell(s->pb);
> +    avio_seek(s->pb, flv->keyframes_info_offset + metadata_size,
> SEEK_SET);
> +
> +    /* start reading at where the keyframe index information will be
> placed */
> +    avio_seek(read_pb, flv->keyframes_info_offset, SEEK_SET);
> +    pos = avio_tell(read_pb);
> +
> +    /* shift data by chunk of at most keyframe infomations size */

I don't understand the above sentence.

[...]
> +    { "build_keyframes_index", "Build keyframes index information into
> metadata", 0, AV_OPT_TYPE_CONST, {.i64 = FLV_BUILD_KEYFRAME_IDX},
> INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "flvflags" },
>      { NULL },
>  };

I would change it to:

"add_keyframe_index", "Add keyframe index metadata"