diff mbox

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

Message ID 1511408256-24718-3-git-send-email-vdixit@akamai.com
State Superseded
Headers show

Commit Message

Dixit, Vishwanath Nov. 23, 2017, 3:37 a.m. UTC 
From: Vishwanath Dixit <vdixit@akamai.com>

Signed-off-by: Karthick J <kjeyapal@akamai.com>
---
 libavformat/hlsenc.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

Comments

Aman Karmani Nov. 23, 2017, 8:19 a.m. UTC | #1 
On Wed, Nov 22, 2017 at 7:38 PM <vdixit@akamai.com> wrote:

> From: Vishwanath Dixit <vdixit@akamai.com>
>
> Signed-off-by: Karthick J <kjeyapal@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;
> +
> +    if (!vid_st && !aud_st) {
> +        av_log(NULL, AV_LOG_WARNING, "Atleast one stream shoud be
> valid\n");
> +        return;
> +    }
> +
> +    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);
> +    }
> +
> +    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 */
> +            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");
> +    }
> +}
> +
>  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];
>
>      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));
> +
> +        if (strlen(vcodec) || strlen(acodec))
> +            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, "\"");
> +
>          if (vs->agroup && aud_st)
>              avio_printf(master_pb, ",AUDIO=\"group_%s\"", vs->agroup);


LGTM.

Have you looked at adding HEVC support?


>
> --
> 1.9.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Jeyapal, Karthick Nov. 23, 2017, 10:32 a.m. UTC | #2 
>From: Aman Gupta <ffmpeg@tmm1.net>


>On Wed, Nov 22, 2017 at 7:38 PM <vdixit@akamai.com> wrote:

>>From: Vishwanath Dixit <vdixit@akamai.com>

>>

>>Signed-off-by: Karthick J <kjeyapal@akamai.com>

>

>LGTM.

>

>Have you looked at adding HEVC support?


Thanks for the reply. 
From our side, there are no immediate plans to add HEVC support.
But after this patch, it should be very simple to add the same.

Regards,
Karthick
Carl Eugen Hoyos Nov. 23, 2017, 10:40 a.m. UTC | #3 
2017-11-23 4:37 GMT+01:00  <vdixit@akamai.com>:
> From: Vishwanath Dixit <vdixit@akamai.com>
>
> Signed-off-by: Karthick J <kjeyapal@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
Jeyapal, Karthick Nov. 24, 2017, 9:54 a.m. UTC | #4 
Since Vishwanath is on leave today, I have made the changes required and have sent patchset v2.

On 11/23/17, 4:11 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote:

>2017-11-23 4:37 GMT+01:00  <vdixit@akamai.com>:

>> From: Vishwanath Dixit <vdixit@akamai.com>

>>

>> Signed-off-by: Karthick J <kjeyapal@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.

Made it based on av_malloc inside the function and caller has to free it. 
Now the code has become much simpler
>

>> +

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

I have removed the warning, as it is a wrong place. 
Retaining the sanity check tough.
>

>> +

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

Couldn’t find any explicit mention for unknown profile. 
https://tools.ietf.org/html/rfc6381

>

>> +

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

No. Also we are not setting constrained_set flags of H264 properly. Have added a TODO there also.
>

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

>


regards,
Karthick
Carl Eugen Hoyos Nov. 24, 2017, 11:11 a.m. UTC | #5 
2017-11-23 4:37 GMT+01:00  <vdixit@akamai.com>:
> From: Vishwanath Dixit <vdixit@akamai.com>

> +    // 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");

What happened to this check?

Carl Eugen
Jeyapal, Karthick Nov. 24, 2017, 11:20 a.m. UTC | #6 
On 11/24/17, 4:41 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote:

>2017-11-23 4:37 GMT+01:00  <vdixit@akamai.com>:

>> From: Vishwanath Dixit <vdixit@akamai.com>


>> +    // 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");

>

>What happened to this check?


It has been handled indirectly in the new function.
In the new function will hit the else condition and “goto fail”
+        } else {
+            goto fail;
+        }

regards,
Karthick
diff mbox

Patch

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;
+
+    if (!vid_st && !aud_st) {
+        av_log(NULL, AV_LOG_WARNING, "Atleast one stream shoud be valid\n");
+        return;
+    }
+
+    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);
+    }
+
+    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 */
+            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");
+    }
+}
+
 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];
 
     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));
+
+        if (strlen(vcodec) || strlen(acodec))
+            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, "\"");
+
         if (vs->agroup && aud_st)
             avio_printf(master_pb, ",AUDIO=\"group_%s\"", vs->agroup);