[FFmpeg-devel] [PATCH] dash: change default MP4 extension to .m4s

[Jeyapal, Karthick]

On 6/20/19 3:00 PM, Alfred E. Heggestad wrote:
>
>
> On 20/06/2019 05:19, Jeyapal, Karthick wrote:
>>
>> On 6/19/19 3:08 PM, Alfred E. Heggestad wrote:
>>> On 19/06/2019 07:21, Jeyapal, Karthick wrote:
>>>>
>>>> On 6/18/19 1:48 PM, Alfred E. Heggestad wrote:
>>>>> On 18/06/2019 04:02, Steven Liu wrote:
>>>>>> Alfred E. Heggestad <alfred.heggestad at gmail.com> 于2019年6月17日周一 下午4:02写道：
>>>>>>>
>>>>>>>     From 923da82598bddd1ed05750427dbc71e607d296a2 Mon Sep 17 00:00:00 2001
>>>>>>> From: "Alfred E. Heggestad" <alfred.heggestad at gmail.com>
>>>>>>> Date: Mon, 17 Jun 2019 09:59:04 +0200
>>>>>>> Subject: [PATCH] dash: change default MP4 extension to .m4s
>>>>>>>
>>>>>>> this was changed in commit 281a21ed50849e3c8c0d03005230e9fd07c24370
>>>>>>> ---
>>>>>>>      libavformat/dashenc.c | 2 +-
>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>>>>>>> index 3fd7e78166..a51a1da0ca 100644
>>>>>>> --- a/libavformat/dashenc.c
>>>>>>> +++ b/libavformat/dashenc.c
>>>>>>> @@ -166,7 +166,7 @@ static struct format_string {
>>>>>>>          const char *str;
>>>>>>>      } formats[] = {
>>>>>>>          { SEGMENT_TYPE_AUTO, "auto" },
>>>>>>> -    { SEGMENT_TYPE_MP4, "mp4" },
>>>>>>> +    { SEGMENT_TYPE_MP4, "m4s" },
>>>>>>>          { SEGMENT_TYPE_WEBM, "webm" },
>>>>>>>          { 0, NULL }
>>>>>>>      };
>>>>>>> -- 
>>>>>>> 2.20.1 (Apple Git-117)
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> ffmpeg-devel mailing list
>>>>>>> ffmpeg-devel at ffmpeg.org
>>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>>
>>>>>>> To unsubscribe, visit link above, or email
>>>>>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe". 
>>>>>>
>>>>>>
>>>>>>
>>>>>> LGTM
>>>>>>
>>>>>
>>>>> the background for this is the extension for DASH media files
>>>>> used to be *.m4s and it is now *.mp4
>>>>>
>>>>>
>>>>> the patch is a suggestion and should be checked by the DASH experts
>>>>>
>>>>> what is correct according to the standard ?
>>>>>
>>>>> the media-file is not really an .mp4 file, it cannot be
>>>>> played with e.g. ffplay:
>>>>>
>>>>>     $ ffplay chunk-stream1-00001.m4s 
>>>> Thanks for submitting the patch. I agree that m4s should be extension for media segments.
>>>> mp4 should be used only for complete files.
>>>> With respect to the patch, dashenc generates either multiple segments or a single file(with byte range as segments) based on "single_file" option.
>>>> The default of mp4 is correct when "single_file" is enabled. But it is wrong when "single_file" is disabled. The proposed patch just reverses this situation.
>>>> I would suggest the patch should handle both cases correctly. 
>>>
>>> Hi,
>>>
>>> many thanks for your review comments.
>>>
>>> I have updated the patch based on your comments, please see below.
>>>
>>>
>>> this code works in my application (both single and multi files)
>>> but the code should be reviewed by someone who has better
>>> knowledge with the code. 
>> Thanks for sending a revised patch promptly.
>> I think your patch below might adversely affect the following code
>>              avio_printf(out, "\t\t\t<Representation id=\"%d\" mimeType=\"video/%s\" codecs=\"%s\"%s width=\"%d\" height=\"%d\"",
>>                  i, os->format_name, os->codec_str, bandwidth_str, s->streams[i]->codecpar->width, s->streams[i]->codecpar->height);
>>
>> mimetype will become "video/m4s" for if the file extension is m4s. But I am not sure if it is correct or if such a mimetype exists.
>> I guess mimetype should remain as "video/mp4" even if the file extension is m4s.
>> Please let me know your views on this.
>>
>
> I tested with ffmpeg 4.1.3 and the mimetype is video/mp4
>
>
> <Representation id="0" mimeType="video/mp4" codecs="avc1.4d401f" bandwidth="1011342" width="1280" height="720">
>
>
>
> I made a new patch which is a bit more readable. Please see here:
>
>
>
> From c90254066e08a8dc46f275fbc2a1d65f26608bd4 Mon Sep 17 00:00:00 2001
> From: "Alfred E. Heggestad" <alfred.heggestad at gmail.com>
> Date: Thu, 20 Jun 2019 11:27:53 +0200
> Subject: [PATCH] dash: split extension for MP4 into .mp4 or .m4s
>
> ---
>  libavformat/dashenc.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index 3fd7e78166..b25afb40aa 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -89,6 +89,7 @@ typedef struct OutputStream {
>      int bit_rate;
>      SegmentType segment_type;  /* segment type selected for this particular stream */
>      const char *format_name;
> +    const char *extension_name;
>      const char *single_file_name;  /* file names selected for this particular stream */
>      const char *init_seg_name;
>      const char *media_seg_name;
> @@ -217,6 +218,16 @@ static const char *get_format_str(SegmentType segment_type) {
>      return NULL;
>  }
>
> +static const char *get_extension_str(SegmentType type, int single_file)
> +{
> +    switch (type) {
> +
> +    case SEGMENT_TYPE_MP4:  return single_file ? "mp4" : "m4s";
> +    case SEGMENT_TYPE_WEBM: return "webm";
> +    default: return NULL;
> +    }
> +}
> +
>  static int handle_io_open_error(AVFormatContext *s, int err, char *url) {
>      DASHContext *c = s->priv_data;
>      char errbuf[AV_ERROR_MAX_STRING_SIZE];
> @@ -254,6 +265,12 @@ static int init_segment_types(AVFormatContext *s)
>              av_log(s, AV_LOG_ERROR, "Could not select DASH segment type for stream %d\n", i);
>              return AVERROR_MUXER_NOT_FOUND;
>          }
> +        os->extension_name = get_extension_str(segment_type, c->single_file);
> +        if (!os->extension_name) {
> +            av_log(s, AV_LOG_ERROR, "Could not get extension type for stream %d\n", i);
> +            return AVERROR_MUXER_NOT_FOUND;
> +        }
> +
>          has_mp4_streams |= segment_type == SEGMENT_TYPE_MP4;
>      }
>
> @@ -1179,17 +1196,17 @@ static int dash_init(AVFormatContext *s)
>              return AVERROR(ENOMEM);
>
>          if (c->init_seg_name) {
> -            os->init_seg_name = av_strireplace(c->init_seg_name, "$ext$", os->format_name);
> +            os->init_seg_name = av_strireplace(c->init_seg_name, "$ext$", os->extension_name);
>              if (!os->init_seg_name)
>                  return AVERROR(ENOMEM);
>          }
>          if (c->media_seg_name) {
> -            os->media_seg_name = av_strireplace(c->media_seg_name, "$ext$", os->format_name);
> +            os->media_seg_name = av_strireplace(c->media_seg_name, "$ext$", os->extension_name);
>              if (!os->media_seg_name)
>                  return AVERROR(ENOMEM);
>          }
>          if (c->single_file_name) {
> -            os->single_file_name = av_strireplace(c->single_file_name, "$ext$", os->format_name);
> +            os->single_file_name = av_strireplace(c->single_file_name, "$ext$", os->extension_name);
>              if (!os->single_file_name)
>                  return AVERROR(ENOMEM);
>          }
Thanks. This new patch looks fine. I wanted to apply it. But I am getting the following error.

Applying: dash: split extension for MP4 into .mp4 or .m4s
error: patch failed: libavformat/dashenc.c:89
error: libavformat/dashenc.c: patch does not apply
Patch failed at 0001 dash: split extension for MP4 into .mp4 or .m4s

Could you please rebase the patch to the latest master and resend it patch as an attachment? 
Also, in this new patch please mention 'dashenc' instead 'dash' in the commit message/subject.

Regards,
Karthick