diff mbox

[FFmpeg-devel] avformat/dashenc: only write video streams into HLS master playlist

Message ID 20180914070817.86135-1-yangjian0911@gmail.com
State Accepted
Commit f7affc6f70cc1bbeec51b1d699e2f17f4ced7362
Headers show

Commit Message

Jian Yang Sept. 14, 2018, 7:08 a.m. UTC 
Tool mediastreamvalidator reports error "Variant media_[N].m3u8 is
missing audio group" for audio streams in HLS master playlist. As audio
streams are already listed in audio group, skip them as variant media
streams in master playlist.
---
 libavformat/dashenc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jeyapal, Karthick Sept. 14, 2018, 8:47 a.m. UTC | #1 
On 9/14/18 12:38 PM, Jian Yang wrote:
> Tool mediastreamvalidator reports error "Variant media_[N].m3u8 is

> missing audio group" for audio streams in HLS master playlist. As audio

> streams are already listed in audio group, skip them as variant media

> streams in master playlist.

Skipping the audio stream altogether is not a good idea. 
Because somebody might want to play an audio-only stream.
One possible fix could be to add the missing audio group, for audio streams as well.
Or maybe the mediastreamvalidator tool is wrong as the spec doesn't mandate the presence of AUDIO group in all variants.
> ---

>  libavformat/dashenc.c | 4 +++-

>  1 file changed, 3 insertions(+), 1 deletion(-)

>

> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c

> index 87e31e25fc..45763301db 100644

> --- a/libavformat/dashenc.c

> +++ b/libavformat/dashenc.c

> @@ -911,8 +911,10 @@ static int write_manifest(AVFormatContext *s, int final)

>              OutputStream *os = &c->streams[i];

>              char *agroup = NULL;

>              int stream_bitrate = st->codecpar->bit_rate + os->muxer_overhead;

> +            if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO)

> +                continue;

>              av_strlcpy(codec_str, os->codec_str, sizeof(codec_str));

> -            if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) && max_audio_bitrate) {

> +            if (max_audio_bitrate) {

>                  agroup = (char *)audio_group;

>                  stream_bitrate += max_audio_bitrate;

>                  av_strlcat(codec_str, ",", sizeof(codec_str));
Jian Yang Sept. 14, 2018, 12:50 p.m. UTC | #2 
Agree! Audio-only stream should be supported.

Just tested adding the missed audio group for audio stream. Tool
mediastreamvalidator doesn't report error message any more, and the
playlist can be played in Safari.

Could you please help to add audio group for audio streams? I can update
the patch if you prefer me to do it.

Thank you very much!


Jeyapal, Karthick <kjeyapal@akamai.com> 于2018年9月14日周五 下午4:47写道：

>
> On 9/14/18 12:38 PM, Jian Yang wrote:
> > Tool mediastreamvalidator reports error "Variant media_[N].m3u8 is
> > missing audio group" for audio streams in HLS master playlist. As audio
> > streams are already listed in audio group, skip them as variant media
> > streams in master playlist.
> Skipping the audio stream altogether is not a good idea.
> Because somebody might want to play an audio-only stream.
> One possible fix could be to add the missing audio group, for audio
> streams as well.
> Or maybe the mediastreamvalidator tool is wrong as the spec doesn't
> mandate the presence of AUDIO group in all variants.
> > ---
> >  libavformat/dashenc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> > index 87e31e25fc..45763301db 100644
> > --- a/libavformat/dashenc.c
> > +++ b/libavformat/dashenc.c
> > @@ -911,8 +911,10 @@ static int write_manifest(AVFormatContext *s, int
> final)
> >              OutputStream *os = &c->streams[i];
> >              char *agroup = NULL;
> >              int stream_bitrate = st->codecpar->bit_rate +
> os->muxer_overhead;
> > +            if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO)
> > +                continue;
> >              av_strlcpy(codec_str, os->codec_str, sizeof(codec_str));
> > -            if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) &&
> max_audio_bitrate) {
> > +            if (max_audio_bitrate) {
> >                  agroup = (char *)audio_group;
> >                  stream_bitrate += max_audio_bitrate;
> >                  av_strlcat(codec_str, ",", sizeof(codec_str));
>
>
Jeyapal, Karthick Sept. 15, 2018, 12:16 p.m. UTC | #3 
On 9/14/18 6:20 PM, Jian Yang wrote:
> Agree! Audio-only stream should be supported.

>

> Just tested adding the missed audio group for audio stream. Tool

> mediastreamvalidator doesn't report error message any more, and the

> playlist can be played in Safari.

Yes, I observed it too. But it throws a different error when there are two audio-only streams with different bitrates.
For example,
#EXTM3U
#EXT-X-VERSION:7
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="group_A1",NAME="audio_1",DEFAULT=YES,URI="media_1.m3u8"
#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="group_A1",NAME="audio_2",DEFAULT=NO,URI="media_2.m3u8"
#EXT-X-STREAM-INF:BANDWIDTH=6195622,RESOLUTION=640x360,CODECS="vp09.00.21.08,mp4a.40.2",AUDIO="group_A1"
media_0.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=129792,CODECS="mp4a.40.2",AUDIO="group_A1"
media_1.m3u8
#EXT-X-STREAM-INF:BANDWIDTH=193792,CODECS="mp4a.40.2",AUDIO="group_A1"
media_2.m3u8

Error: Measured peak bitrate compared to master playlist declared value exceeds error tolerance
--> Detail:  Measured: 195.73 kb/s, Master playlist: 129.79 kb/s, Error: 33.69%, Combined rendition name: audio_2
--> Compare: media_1.m3u8

>

> Could you please help to add audio group for audio streams? I can update

> the patch if you prefer me to do it.

Do you really need to remove that mediastreamvalidator error. Because Safari, hls.js and other popular players are playing out properly even with that mediastreamvalidator error. 

If you really want to remove that mediastreamvalidator error, one option it to slightly modify your original patch and add a command line option for removing audio-only streams. In that way you could remove that audio-only stream from master playlist by enabling that option, while leaving the original behavior untouched for others. Please let me know if you any more clarifications and/or help. 
>

> Thank you very much!

>

>

> Jeyapal, Karthick <kjeyapal@akamai.com> 于2018年9月14日周五 下午4:47写道：

>

>>

>> On 9/14/18 12:38 PM, Jian Yang wrote:

>>> Tool mediastreamvalidator reports error "Variant media_[N].m3u8 is

>>> missing audio group" for audio streams in HLS master playlist. As audio

>>> streams are already listed in audio group, skip them as variant media

>>> streams in master playlist.

>> Skipping the audio stream altogether is not a good idea.

>> Because somebody might want to play an audio-only stream.

>> One possible fix could be to add the missing audio group, for audio

>> streams as well.

>> Or maybe the mediastreamvalidator tool is wrong as the spec doesn't

>> mandate the presence of AUDIO group in all variants.

>>> ---

>>>  libavformat/dashenc.c | 4 +++-

>>>  1 file changed, 3 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c

>>> index 87e31e25fc..45763301db 100644

>>> --- a/libavformat/dashenc.c

>>> +++ b/libavformat/dashenc.c

>>> @@ -911,8 +911,10 @@ static int write_manifest(AVFormatContext *s, int

>> final)

>>>              OutputStream *os = &c->streams[i];

>>>              char *agroup = NULL;

>>>              int stream_bitrate = st->codecpar->bit_rate +

>> os->muxer_overhead;

>>> +            if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO)

>>> +                continue;

>>>              av_strlcpy(codec_str, os->codec_str, sizeof(codec_str));

>>> -            if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) &&

>> max_audio_bitrate) {

>>> +            if (max_audio_bitrate) {

>>>                  agroup = (char *)audio_group;

>>>                  stream_bitrate += max_audio_bitrate;

>>>                  av_strlcat(codec_str, ",", sizeof(codec_str));

>>

>>

>
Jeyapal, Karthick Oct. 11, 2018, 7:31 a.m. UTC | #4 
On 9/14/18 12:38 PM, Jian Yang wrote:
> Tool mediastreamvalidator reports error "Variant media_[N].m3u8 is

> missing audio group" for audio streams in HLS master playlist. As audio

> streams are already listed in audio group, skip them as variant media

> streams in master playlist.

> ---

>  libavformat/dashenc.c | 4 +++-

>  1 file changed, 3 insertions(+), 1 deletion(-)

>

> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c

> index 87e31e25fc..45763301db 100644

> --- a/libavformat/dashenc.c

> +++ b/libavformat/dashenc.c

> @@ -911,8 +911,10 @@ static int write_manifest(AVFormatContext *s, int final)

>              OutputStream *os = &c->streams[i];

>              char *agroup = NULL;

>              int stream_bitrate = st->codecpar->bit_rate + os->muxer_overhead;

> +            if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO)

> +                continue;

>              av_strlcpy(codec_str, os->codec_str, sizeof(codec_str));

> -            if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) && max_audio_bitrate) {

> +            if (max_audio_bitrate) {

>                  agroup = (char *)audio_group;

>                  stream_bitrate += max_audio_bitrate;

>                  av_strlcat(codec_str, ",", sizeof(codec_str));

Pushed this patch. Thanks.
In spite of my original objections for supporting audio-only, I am pushing this patch as I am hearing reports of HLS master playlist generated being not playable in iOS devices due to this issue.
Hence pushing this patch becomes important for supporting basic HLS playback is more important than supporting audio-only HLS stream.

Regards,
Karthick
diff mbox

Patch

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index 87e31e25fc..45763301db 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -911,8 +911,10 @@  static int write_manifest(AVFormatContext *s, int final)
             OutputStream *os = &c->streams[i];
             char *agroup = NULL;
             int stream_bitrate = st->codecpar->bit_rate + os->muxer_overhead;
+            if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO)
+                continue;
             av_strlcpy(codec_str, os->codec_str, sizeof(codec_str));
-            if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) && max_audio_bitrate) {
+            if (max_audio_bitrate) {
                 agroup = (char *)audio_group;
                 stream_bitrate += max_audio_bitrate;
                 av_strlcat(codec_str, ",", sizeof(codec_str));