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

[Jeyapal, Karthick]

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 at gmail.com> wrote:

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