diff mbox

[FFmpeg-devel] This fixes ISO date formatissue when manifest created by this muxer is not playable in most players. This ensures compatibility with dash standard. Tested on many players (dashj.js, shaka, VLC, etc.)

Message ID 20170426112759.12130-1-mfojtak@seznam.cz
State New
Headers show

Commit Message

mfojtak April 26, 2017, 11:27 a.m. UTC 
---
 libavformat/dashenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Aaron Levinson April 30, 2017, 6:47 p.m. UTC | #1 
On 4/26/2017 4:27 AM, mfojtak wrote:
> ---
>  libavformat/dashenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index 6232c70..fe1d6c2 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -433,7 +433,7 @@ static void format_date_now(char *buf, int size)
>      struct tm *ptm, tmbuf;
>      ptm = gmtime_r(&t, &tmbuf);
>      if (ptm) {
> -        if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S", ptm))
> +        if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%SZ", ptm))
>              buf[0] = '\0';
>      }
>  }
>

This change appears to be correct.  I wasn't previously knowledgeable 
about the 'Z' suffix, but I looked into it and it is documented in ISO 
8601 (https://en.wikipedia.org/wiki/ISO_8601 and also 
http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15 ).

On a separate note, the actual format is:  YYYY-MM-DDTHH:mm:ss.sssZ . 
The ".sss" part is missing from this implementation, which represents 
milliseconds.  According to the specification, ".sss" may be absent, but 
maybe it would work with even more players if it were included. 
Technically, the specification states that an absent time-zone offset 
should be treated as 'Z', which indicates that the code was already 
compliant without the 'Z', even if it didn't work with most players. 
strftime() doesn't handle milliseconds, but perhaps it ought to populate 
milliseconds anyway as follows:

         if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S.000Z", ptm))

Aaron Levinson
mfojtak May 2, 2017, 6:06 a.m. UTC | #2 
Currently this muxer does not work at all. I don't know if 000Z would make 
it compatible with more player as I don't know any. However, adding Z makes 
it compatible with most popular ones like dash.js and shaka. Having this 
muxer working properly would bring more attention to it and maybe in the 
future somebody tests it with more players. But for now I suggest to roll 
out the change and "unblock" this muxer for some real wold use. Also, it 
would be great to make this muxer codec and container agnostic as it works 
with h264 and mp4 only. But again, nobody would bother if the muxer doesn't 
work at all with browsers. 
 ---------- Původní e-mail ----------
Od: Aaron Levinson <alevinsn@aracnet.com>
Komu: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Datum: 30. 4. 2017 20:47:59
Předmět: Re: [FFmpeg-devel] [PATCH] This fixes ISO date formatissue when 
manifest created by this muxer is not playable in most players. This ensures
compatibility with dash standard. Tested on many players (dashj.js, shaka, 
VLC, etc.) 
"On 4/26/2017 4:27 AM, mfojtak wrote: 
> --- 
> libavformat/dashenc.c | 2 +- 
> 1 file changed, 1 insertion(+), 1 deletion(-) 
> 
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c 
> index 6232c70..fe1d6c2 100644 
> --- a/libavformat/dashenc.c 
> +++ b/libavformat/dashenc.c 
> @@ -433,7 +433,7 @@ static void format_date_now(char *buf, int size) 
> struct tm *ptm, tmbuf; 
> ptm = gmtime_r(&t, &tmbuf); 
> if (ptm) { 
> - if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S", ptm)) 
> + if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%SZ", ptm)) 
> buf[0] = '\0'; 
> } 
> } 
> 

This change appears to be correct. I wasn't previously knowledgeable 
about the 'Z' suffix, but I looked into it and it is documented in ISO 
8601 (https://en.wikipedia.org/wiki/ISO_8601 and also 
http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15 ). 

On a separate note, the actual format is: YYYY-MM-DDTHH:mm:ss.sssZ . 
The ".sss" part is missing from this implementation, which represents 
milliseconds. According to the specification, ".sss" may be absent, but 
maybe it would work with even more players if it were included. 
Technically, the specification states that an absent time-zone offset 
should be treated as 'Z', which indicates that the code was already 
compliant without the 'Z', even if it didn't work with most players. 
strftime() doesn't handle milliseconds, but perhaps it ought to populate 
milliseconds anyway as follows: 

if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S.000Z", ptm)) 

Aaron Levinson 
"
Aaron Levinson May 2, 2017, 9:17 p.m. UTC | #3 
On 5/1/2017 11:06 PM, MFojtak wrote:
> Currently this muxer does not work at all. I don't know if 000Z would make
> it compatible with more player as I don't know any. However, adding Z makes
> it compatible with most popular ones like dash.js and shaka. Having this
> muxer working properly would bring more attention to it and maybe in the
> future somebody tests it with more players. But for now I suggest to roll
> out the change and "unblock" this muxer for some real wold use. Also, it
> would be great to make this muxer codec and container agnostic as it works
> with h264 and mp4 only. But again, nobody would bother if the muxer doesn't
> work at all with browsers.

I think your original patch is fine, and I only offered the possibility 
that adding ".000Z" might be even better than just "Z".  I don't have 
push privileges, so I can't commit your patch, but hopefully someone 
else will do so.

Aaron Levinson
wm4 May 2, 2017, 9:29 p.m. UTC | #4 
On Tue, 2 May 2017 14:17:33 -0700
Aaron Levinson <alevinsn@aracnet.com> wrote:

> On 5/1/2017 11:06 PM, MFojtak wrote:
> > Currently this muxer does not work at all. I don't know if 000Z would make
> > it compatible with more player as I don't know any. However, adding Z makes
> > it compatible with most popular ones like dash.js and shaka. Having this
> > muxer working properly would bring more attention to it and maybe in the
> > future somebody tests it with more players. But for now I suggest to roll
> > out the change and "unblock" this muxer for some real wold use. Also, it
> > would be great to make this muxer codec and container agnostic as it works
> > with h264 and mp4 only. But again, nobody would bother if the muxer doesn't
> > work at all with browsers.  
> 
> I think your original patch is fine, and I only offered the possibility 
> that adding ".000Z" might be even better than just "Z".  I don't have 
> push privileges, so I can't commit your patch, but hopefully someone 
> else will do so.

Before someone pushes it, please fix the commit message.
Aaron Levinson May 5, 2017, 5:50 a.m. UTC | #5 
On 5/2/2017 2:29 PM, wm4 wrote:
> On Tue, 2 May 2017 14:17:33 -0700
> Aaron Levinson <alevinsn@aracnet.com> wrote:
>
>> On 5/1/2017 11:06 PM, MFojtak wrote:
>>> Currently this muxer does not work at all. I don't know if 000Z would make
>>> it compatible with more player as I don't know any. However, adding Z makes
>>> it compatible with most popular ones like dash.js and shaka. Having this
>>> muxer working properly would bring more attention to it and maybe in the
>>> future somebody tests it with more players. But for now I suggest to roll
>>> out the change and "unblock" this muxer for some real wold use. Also, it
>>> would be great to make this muxer codec and container agnostic as it works
>>> with h264 and mp4 only. But again, nobody would bother if the muxer doesn't
>>> work at all with browsers.
>>
>> I think your original patch is fine, and I only offered the possibility
>> that adding ".000Z" might be even better than just "Z".  I don't have
>> push privileges, so I can't commit your patch, but hopefully someone
>> else will do so.
>
> Before someone pushes it, please fix the commit message.

MFojtak:  you can make it more likely that this patch will be pushed 
sooner by altering the commit message (shortening it--plenty of examples 
on the e-mail list) and then putting any additional information later. 
Something like:

"
avformat/dashenc:  Adjusts ISO date formatting for improved 
compatibility with most players

Adjusts ISO date formatting....
"

Aaron Levinson
mfojtak May 10, 2017, 7:22 a.m. UTC | #6 
Hello,



Does anybody know what happened with this patch?


---------- Původní e-mail ----------
Od: MFojtak <MFojtak@seznam.cz>
Komu: Aaron Levinson <alevinsn@aracnet.com>, FFmpeg development discussions 
and patches <ffmpeg-devel@ffmpeg.org>
Datum: 2. 5. 2017 8:06:29
Předmět: Re: [FFmpeg-devel] [PATCH] This fixes ISO date formatissue when 
manifest created by this muxer is not playable in most players. This ensures
compatibility with dash standard. Tested on many players (dashj.js, shaka, 
VLC, etc.) 
"Currently this muxer does not work at all. I don't know if 000Z would make 
it compatible with more player as I don't know any. However, adding Z makes 
it compatible with most popular ones like dash.js and shaka. Having this 
muxer working properly would bring more attention to it and maybe in the 
future somebody tests it with more players. But for now I suggest to roll 
out the change and "unblock" this muxer for some real wold use. Also, it 
would be great to make this muxer codec and container agnostic as it works 
with h264 and mp4 only. But again, nobody would bother if the muxer doesn't 
work at all with browsers. 
 ---------- Původní e-mail ----------
Od: Aaron Levinson <alevinsn@aracnet.com>
Komu: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Datum: 30. 4. 2017 20:47:59
Předmět: Re: [FFmpeg-devel] [PATCH] This fixes ISO date formatissue when 
manifest created by this muxer is not playable in most players. This ensures
compatibility with dash standard. Tested on many players (dashj.js, shaka, 
VLC, etc.) 
"On 4/26/2017 4:27 AM, mfojtak wrote: 
> --- 
> libavformat/dashenc.c | 2 +- 
> 1 file changed, 1 insertion(+), 1 deletion(-) 
> 
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c 
> index 6232c70..fe1d6c2 100644 
> --- a/libavformat/dashenc.c 
> +++ b/libavformat/dashenc.c 
> @@ -433,7 +433,7 @@ static void format_date_now(char *buf, int size) 
> struct tm *ptm, tmbuf; 
> ptm = gmtime_r(&t, &tmbuf); 
> if (ptm) { 
> - if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S", ptm)) 
> + if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%SZ", ptm)) 
> buf[0] = '\0'; 
> } 
> } 
> 

This change appears to be correct. I wasn't previously knowledgeable 
about the 'Z' suffix, but I looked into it and it is documented in ISO 
8601 (https://en.wikipedia.org/wiki/ISO_8601 and also 
http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15 ). 

On a separate note, the actual format is: YYYY-MM-DDTHH:mm:ss.sssZ . 
The ".sss" part is missing from this implementation, which represents 
milliseconds. According to the specification, ".sss" may be absent, but 
maybe it would work with even more players if it were included. 
Technically, the specification states that an absent time-zone offset 
should be treated as 'Z', which indicates that the code was already 
compliant without the 'Z', even if it didn't work with most players. 
strftime() doesn't handle milliseconds, but perhaps it ought to populate 
milliseconds anyway as follows: 

if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S.000Z", ptm)) 

Aaron Levinson 
""
diff mbox

Patch

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index 6232c70..fe1d6c2 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -433,7 +433,7 @@  static void format_date_now(char *buf, int size)
     struct tm *ptm, tmbuf;
     ptm = gmtime_r(&t, &tmbuf);
     if (ptm) {
-        if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%S", ptm))
+        if (!strftime(buf, size, "%Y-%m-%dT%H:%M:%SZ", ptm))
             buf[0] = '\0';
     }
 }