[FFmpeg-devel] [PATCH 3/3] avformat/hlsenc:addition of CODECS attribute in the master playlist

[Carl Eugen Hoyos]

2017-11-23 4:37 GMT+01:00  <vdixit at akamai.com>:
> From: Vishwanath Dixit <vdixit at akamai.com>
>
> Signed-off-by: Karthick J <kjeyapal at akamai.com>
> ---
>  libavformat/hlsenc.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 9fed6a3..32246c4 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -1065,6 +1065,52 @@ static int get_relative_url(const char *master_url, const char *media_url,
>      return 0;
>  }
>
> +static void get_codec_str(AVStream *vid_st, AVStream *aud_st, char *vcodec,
> +                          char *acodec, int vcodec_len, int acodec_len) {

> +    if (vcodec_len > 0)
> +        vcodec[0] = '\0';
> +    else
> +        return;
> +
> +    if (acodec_len > 0)
> +        acodec[0] = '\0';
> +    else
> +        return;

What are these supposed to do?
Actually: Please add a line below to avoid these.

> +
> +    if (!vid_st && !aud_st) {
> +        av_log(NULL, AV_LOG_WARNING, "Atleast one stream shoud be valid\n");
> +        return;
> +    }

This looks like the wrong place for such a check.

> +
> +    if (vid_st && vid_st->codecpar->profile != FF_PROFILE_UNKNOWN &&
> +        vid_st->codecpar->level != FF_LEVEL_UNKNOWN &&
> +        vid_st->codecpar->codec_id == AV_CODEC_ID_H264) {
> +        snprintf(vcodec, vcodec_len, "avc1.%02x00%02x",
> +                vid_st->codecpar->profile, vid_st->codecpar->level);
> +    }

The rfc does not specify a string for unknown profile?

> +
> +    if (aud_st) {
> +        if (aud_st->codecpar->codec_id == AV_CODEC_ID_MP2) {
> +            snprintf(acodec, acodec_len, "mp4a.40.33");
> +        } else if (aud_st->codecpar->codec_id == AV_CODEC_ID_MP3) {
> +            snprintf(acodec, acodec_len, "mp4a.40.34");
> +        } else if (aud_st->codecpar->codec_id == AV_CODEC_ID_AAC) {

> +            /* TODO : For HE-AAC, HE-AACv2, the last digit needs to be set to 5 and 29 respectively */

Yes.
Is this the only special case already known?

> +            snprintf(acodec, acodec_len, "mp4a.40.2");
> +        } else if (aud_st->codecpar->codec_id == AV_CODEC_ID_AC3) {
> +            snprintf(acodec, acodec_len, "mp4a.A5");
> +        } else if (aud_st->codecpar->codec_id == AV_CODEC_ID_EAC3) {
> +            snprintf(acodec, acodec_len, "mp4a.A6");
> +        }
> +    }
> +
> +    // either provide codec string for both active streams or for none
> +    if (vid_st && aud_st && (!strlen(vcodec) || !strlen(acodec))) {
> +        acodec[0] = vcodec[0] = '\0';
> +        av_log(NULL, AV_LOG_INFO, "Codec string not available for audio or video stream\n");
> +    }

This needs a context and maybe a higher verbosity.

> +}
> +
>  static int create_master_playlist(AVFormatContext *s,
>                                    VariantStream * const input_vs)
>  {
> @@ -1075,7 +1121,7 @@ static int create_master_playlist(AVFormatContext *s,
>      AVDictionary *options = NULL;
>      unsigned int i, j;
>      int m3u8_name_size, ret, bandwidth;
> -    char *m3U8_rel_name;
> +    char *m3U8_rel_name, vcodec[32], acodec[32];

I suspect you should initialize the first byte here to
avoid the check on top.

>
>      input_vs->m3u8_created = 1;
>      if (!hls->master_m3u8_created) {
> @@ -1203,6 +1249,25 @@ static int create_master_playlist(AVFormatContext *s,
>              avio_printf(master_pb, ",RESOLUTION=%dx%d", vid_st->codecpar->width,
>                      vid_st->codecpar->height);
>
> +        get_codec_str(vid_st, aud_st, vcodec, acodec, sizeof(vcodec),
> +                      sizeof(acodec));

Imo, use defines instead of sizeof() here.

> +        if (strlen(vcodec) || strlen(acodec))

Even if not speed-critical code, checking the first byte
may be simpler.

Using "{" here simplifies the code below.

> +            avio_printf(master_pb, ",CODECS=\"");
> +
> +        if (strlen(vcodec)) {
> +            avio_printf(master_pb, "%s", vcodec);
> +
> +            if (strlen(acodec))
> +                avio_printf(master_pb, ",");
> +        }
> +
> +        if (strlen(acodec))
> +            avio_printf(master_pb, "%s", acodec);
> +
> +        if (strlen(vcodec) || strlen(acodec))
> +            avio_printf(master_pb, "\"");

Carl Eugen