diff mbox

[FFmpeg-devel,HLS] Add LANGUAGE attribute to #EXT-X-MEDIA tag for audio-only variant streams.

Message ID CACS3z+YrZMi_3n49PDyUx4qG8OZ=FOuQk6YRqnyS2WoinGVodg@mail.gmail.com
State Superseded
Headers show

Commit Message

Philippe Symons Nov. 19, 2018, 8:21 p.m. UTC 
Hello everyone,

I've updated the patch based on the feedback from Moritz. Thanks, btw! I
apologize if I wasted your time with this.

I've updated the patch based on your feedback. I hope I got it right this
time.

Looking forward to your feedback,

Best regards,

Philippe Symons

Op za 17 nov. 2018 om 11:20 schreef Moritz Barsnick <barsnick@gmx.net>:

> On Thu, Nov 15, 2018 at 00:29:00 +0100, Philippe Symons wrote:
> > Here is the new version of the patch in which the comments on the curly
> > braces have been resolved as well.
>
> Style-wise, there are other issues. (I'm not judging technically here.)
>
> > Subject: [PATCH] [HLS] Add LANGUAGE attribute to #EXT-X-MEDIA tag for
> >  audio-only variant streams.
>
> The project prefers:
> avformat/hls,dash: add LANGUAGE attribute to #EXT-X-MEDIA tag for
> audio-only variant streams
>
> (i.e. source hierarchy, no trailing dot, ...)
>
> >      set_http_options(s, &options, hls);
> > -
> >      ret = hlsenc_io_open(s, &hls->m3u8_out, hls->master_m3u8_url,
> &options);
>
> Don't add extra lines, please.
>
> >      for (i = 0; i < hls->nb_varstreams; i++) {
> > +        AVFormatContext* var_context;
> > +        char* language = NULL;
>
> Please read
> https://www.ffmpeg.org/developer.html#Coding-Rules-1
>
> abouts brackets and indentation, and look at other code sections..
> Furthermore, the "pointer specifier" (or whatever that's called) sticks
> to the variable, not the type, in ffmpeg declarations:
>
> char *language = NULL;
>
> Same in other places.
>
> > +        if(var_context && var_context->nb_streams > 0) {
>
> Bracket / whitspace style: "if (var_context [...]"
>
> > +            if(langEntry) {
> Same here: if (langEntry)
> And in other places.
>
> > +                language = langEntry->value;
> > +            }
>
> Some developers prefer you to drop the curly brackets around one-liner
> blocks.
>
> No review of the technical implications, sorry.
>
> Cheers,
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Comments

Philippe Symons Nov. 19, 2018, 8:44 p.m. UTC | #1 
... I just noticed the langEntry var name. Here's an update. Sorry... Even
after checking the guidelines twice, apparently I still miss things.

I'll try to do better next time. I really expect this to be the last
update. (at the very least concerning coding style issues)

Sorry,

Philippe

Op ma 19 nov. 2018 om 21:21 schreef Philippe Symons <
philippe.symons@gmail.com>:

> Hello everyone,
>
> I've updated the patch based on the feedback from Moritz. Thanks, btw! I
> apologize if I wasted your time with this.
>
> I've updated the patch based on your feedback. I hope I got it right this
> time.
>
> Looking forward to your feedback,
>
> Best regards,
>
> Philippe Symons
>
> Op za 17 nov. 2018 om 11:20 schreef Moritz Barsnick <barsnick@gmx.net>:
>
>> On Thu, Nov 15, 2018 at 00:29:00 +0100, Philippe Symons wrote:
>> > Here is the new version of the patch in which the comments on the curly
>> > braces have been resolved as well.
>>
>> Style-wise, there are other issues. (I'm not judging technically here.)
>>
>> > Subject: [PATCH] [HLS] Add LANGUAGE attribute to #EXT-X-MEDIA tag for
>> >  audio-only variant streams.
>>
>> The project prefers:
>> avformat/hls,dash: add LANGUAGE attribute to #EXT-X-MEDIA tag for
>> audio-only variant streams
>>
>> (i.e. source hierarchy, no trailing dot, ...)
>>
>> >      set_http_options(s, &options, hls);
>> > -
>> >      ret = hlsenc_io_open(s, &hls->m3u8_out, hls->master_m3u8_url,
>> &options);
>>
>> Don't add extra lines, please.
>>
>> >      for (i = 0; i < hls->nb_varstreams; i++) {
>> > +        AVFormatContext* var_context;
>> > +        char* language = NULL;
>>
>> Please read
>> https://www.ffmpeg.org/developer.html#Coding-Rules-1
>>
>> abouts brackets and indentation, and look at other code sections..
>> Furthermore, the "pointer specifier" (or whatever that's called) sticks
>> to the variable, not the type, in ffmpeg declarations:
>>
>> char *language = NULL;
>>
>> Same in other places.
>>
>> > +        if(var_context && var_context->nb_streams > 0) {
>>
>> Bracket / whitspace style: "if (var_context [...]"
>>
>> > +            if(langEntry) {
>> Same here: if (langEntry)
>> And in other places.
>>
>> > +                language = langEntry->value;
>> > +            }
>>
>> Some developers prefer you to drop the curly brackets around one-liner
>> blocks.
>>
>> No review of the technical implications, sorry.
>>
>> Cheers,
>> Moritz
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
diff mbox

Patch

From 97a9d2ebf2abde9c61cadb355cfe2ca2f960c5c7 Mon Sep 17 00:00:00 2001
From: Philippe Symons <philippe.symons@gmail.com>
Date: Mon, 19 Nov 2018 20:34:36 +0100
Subject: [PATCH] avformat/hls,dash: add LANGUAGE attribute to #EXT-X-MEDIA tag
 for audio-only variant streams

Signed-off-by: Philippe Symons <philippe.symons@gmail.com>
---
 libavformat/dashenc.c     |  2 +-
 libavformat/hlsenc.c      | 20 +++++++++++++++++++-
 libavformat/hlsplaylist.c |  6 +++++-
 libavformat/hlsplaylist.h |  2 +-
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index d151921175..17465c1917 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -898,7 +898,7 @@  static int write_manifest(AVFormatContext *s, int final)
             if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
                 continue;
             get_hls_playlist_name(playlist_file, sizeof(playlist_file), NULL, i);
-            ff_hls_write_audio_rendition(c->m3u8_out, (char *)audio_group,
+            ff_hls_write_audio_rendition(c->m3u8_out, (char *)audio_group, NULL,
                                          playlist_file, i, is_default);
             max_audio_bitrate = FFMAX(st->codecpar->bit_rate +
                                       os->muxer_overhead, max_audio_bitrate);
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 24b678efe0..6b937c244e 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -1228,12 +1228,30 @@  static int create_master_playlist(AVFormatContext *s,
 
     /* For audio only variant streams add #EXT-X-MEDIA tag with attributes*/
     for (i = 0; i < hls->nb_varstreams; i++) {
+        AVFormatContext *var_context;
+        char *language = NULL;
+
         vs = &(hls->var_streams[i]);
+        var_context = vs->avf;
 
         if (vs->has_video || vs->has_subtitle || !vs->agroup)
             continue;
 
         m3u8_name_size = strlen(vs->m3u8_name) + 1;
+        
+        /*
+         * Try to obtain the language code of the audio stream.
+         * -if available- it will be used to write the LANGUAGE
+         * attribute in the #EXT-X-MEDIA tag
+         */
+        if (var_context && var_context->nb_streams > 0) {
+            AVDictionary *meta_dict = vs->streams[0]->metadata;
+            AVDictionaryEntry *langEntry = av_dict_get(meta_dict, "language", NULL, 0);
+            
+            if (langEntry)
+                language = langEntry->value;
+        }
+
         m3u8_rel_name = av_malloc(m3u8_name_size);
         if (!m3u8_rel_name) {
             ret = AVERROR(ENOMEM);
@@ -1247,7 +1265,7 @@  static int create_master_playlist(AVFormatContext *s,
             goto fail;
         }
 
-        ff_hls_write_audio_rendition(hls->m3u8_out, vs->agroup, m3u8_rel_name, 0, 1);
+        ff_hls_write_audio_rendition(hls->m3u8_out, vs->agroup, language, m3u8_rel_name, 0, 1);
 
         av_freep(&m3u8_rel_name);
     }
diff --git a/libavformat/hlsplaylist.c b/libavformat/hlsplaylist.c
index efcbff0009..a01d26a5c0 100644
--- a/libavformat/hlsplaylist.c
+++ b/libavformat/hlsplaylist.c
@@ -35,12 +35,16 @@  void ff_hls_write_playlist_version(AVIOContext *out, int version) {
     avio_printf(out, "#EXT-X-VERSION:%d\n", version);
 }
 
-void ff_hls_write_audio_rendition(AVIOContext *out, char *agroup,
+void ff_hls_write_audio_rendition(AVIOContext *out, char *agroup, char *language,
                                   char *filename, int name_id, int is_default) {
     if (!out || !agroup || !filename)
         return;
 
     avio_printf(out, "#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID=\"group_%s\"", agroup);
+
+    if (language)
+        avio_printf(out, ",LANGUAGE=\"%s\"", language);
+
     avio_printf(out, ",NAME=\"audio_%d\",DEFAULT=%s,URI=\"%s\"\n", name_id,
                      is_default ? "YES" : "NO", filename);
 }
diff --git a/libavformat/hlsplaylist.h b/libavformat/hlsplaylist.h
index 5054b01c8f..351888a184 100644
--- a/libavformat/hlsplaylist.h
+++ b/libavformat/hlsplaylist.h
@@ -37,7 +37,7 @@  typedef enum {
 } PlaylistType;
 
 void ff_hls_write_playlist_version(AVIOContext *out, int version);
-void ff_hls_write_audio_rendition(AVIOContext *out, char *agroup,
+void ff_hls_write_audio_rendition(AVIOContext *out, char *agroup, char *language,
                                   char *filename, int name_id, int is_default);
 void ff_hls_write_stream_info(AVStream *st, AVIOContext *out,
                               int bandwidth, char *filename, char *agroup,
-- 
2.17.1