diff mbox

[FFmpeg-devel,v2] avformat/hlsenc: improve the code readable of 1000000

Message ID 20170123110114.10849-1-lq@chinaffmpeg.org
State Accepted
Commit 2f7cc21b61220d205e3c57384f354187417971fb
Headers show

Commit Message

Liu Steven Jan. 23, 2017, 11:01 a.m. UTC 
Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Bodecs Bela Jan. 23, 2017, 2:08 p.m. UTC | #1 
2017.01.23. 12:01 keltezéssel, Steven Liu írta:
> Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>   libavformat/hlsenc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 85d3955..0170b34 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -47,6 +47,7 @@ typedef enum {
>   
>   #define KEYSIZE 16
>   #define LINE_BUFFER_SIZE 1024
> +#define HLS_MICROSECOND_UNIT   1000000
>   
>   typedef struct HLSSegment {
>       char filename[1024];
> @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext *s, HLSContext *hls, double
>                   return AVERROR(ENOMEM);
>               }
>               if (replace_int_data_in_filename(hls->avf->filename, sizeof(hls->avf->filename),
> -                filename, 't',  (int64_t)round(1000000 * duration)) < 1) {
> +                filename, 't',  (int64_t)round(duration * HLS_MICROSECOND_UNIT)) < 1) {
>                   av_log(hls, AV_LOG_ERROR,
>                          "Invalid second level segment filename template '%s', "
>                           "you can try to remove second_level_segment_time flag\n",
I think it is good.

bb
James Almer Jan. 23, 2017, 2:14 p.m. UTC | #2 
On 1/23/2017 8:01 AM, Steven Liu wrote:
> Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/hlsenc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 85d3955..0170b34 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -47,6 +47,7 @@ typedef enum {
>  
>  #define KEYSIZE 16
>  #define LINE_BUFFER_SIZE 1024
> +#define HLS_MICROSECOND_UNIT   1000000

You could instead use AV_TIME_BASE. It's more in line with the rest of the
codebase.

>  
>  typedef struct HLSSegment {
>      char filename[1024];
> @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext *s, HLSContext *hls, double
>                  return AVERROR(ENOMEM);
>              }
>              if (replace_int_data_in_filename(hls->avf->filename, sizeof(hls->avf->filename),
> -                filename, 't',  (int64_t)round(1000000 * duration)) < 1) {
> +                filename, 't',  (int64_t)round(duration * HLS_MICROSECOND_UNIT)) < 1) {
>                  av_log(hls, AV_LOG_ERROR,
>                         "Invalid second level segment filename template '%s', "
>                          "you can try to remove second_level_segment_time flag\n",
>
Steven Liu Jan. 23, 2017, 2:25 p.m. UTC | #3 
2017-01-23 22:14 GMT+08:00 James Almer <jamrial@gmail.com>:

> On 1/23/2017 8:01 AM, Steven Liu wrote:
> > Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
> > Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> > ---
> >  libavformat/hlsenc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> > index 85d3955..0170b34 100644
> > --- a/libavformat/hlsenc.c
> > +++ b/libavformat/hlsenc.c
> > @@ -47,6 +47,7 @@ typedef enum {
> >
> >  #define KEYSIZE 16
> >  #define LINE_BUFFER_SIZE 1024
> > +#define HLS_MICROSECOND_UNIT   1000000
>
> You could instead use AV_TIME_BASE. It's more in line with the rest of the
> codebase.
>
No, the 1st version, Bodecs Bela has point the 1000000 is not AV_TIME_BASE
mean.

>
> >
> >  typedef struct HLSSegment {
> >      char filename[1024];
> > @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext
> *s, HLSContext *hls, double
> >                  return AVERROR(ENOMEM);
> >              }
> >              if (replace_int_data_in_filename(hls->avf->filename,
> sizeof(hls->avf->filename),
> > -                filename, 't',  (int64_t)round(1000000 * duration)) <
> 1) {
> > +                filename, 't',  (int64_t)round(duration *
> HLS_MICROSECOND_UNIT)) < 1) {
> >                  av_log(hls, AV_LOG_ERROR,
> >                         "Invalid second level segment filename template
> '%s', "
> >                          "you can try to remove
> second_level_segment_time flag\n",
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Jan. 23, 2017, 2:55 p.m. UTC | #4 
On 1/23/2017 11:25 AM, Steven Liu wrote:
> 2017-01-23 22:14 GMT+08:00 James Almer <jamrial@gmail.com>:
> 
>> On 1/23/2017 8:01 AM, Steven Liu wrote:
>>> Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>> ---
>>>  libavformat/hlsenc.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>> index 85d3955..0170b34 100644
>>> --- a/libavformat/hlsenc.c
>>> +++ b/libavformat/hlsenc.c
>>> @@ -47,6 +47,7 @@ typedef enum {
>>>
>>>  #define KEYSIZE 16
>>>  #define LINE_BUFFER_SIZE 1024
>>> +#define HLS_MICROSECOND_UNIT   1000000
>>
>> You could instead use AV_TIME_BASE. It's more in line with the rest of the
>> codebase.
>>
> No, the 1st version, Bodecs Bela has point the 1000000 is not AV_TIME_BASE
> mean.

I'm not sure i understand his reasoning, seeing that AV_TIME_BASE is
defined as 1000000 in avutil.h and i don't see it changing anytime soon,
but it's just a nit so not really important. This patch is good as is.

> 
>>
>>>
>>>  typedef struct HLSSegment {
>>>      char filename[1024];
>>> @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext
>> *s, HLSContext *hls, double
>>>                  return AVERROR(ENOMEM);
>>>              }
>>>              if (replace_int_data_in_filename(hls->avf->filename,
>> sizeof(hls->avf->filename),
>>> -                filename, 't',  (int64_t)round(1000000 * duration)) <
>> 1) {
>>> +                filename, 't',  (int64_t)round(duration *
>> HLS_MICROSECOND_UNIT)) < 1) {
>>>                  av_log(hls, AV_LOG_ERROR,
>>>                         "Invalid second level segment filename template
>> '%s', "
>>>                          "you can try to remove
>> second_level_segment_time flag\n",
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Bodecs Bela Jan. 23, 2017, 3:10 p.m. UTC | #5 
2017.01.23. 15:55 keltezéssel, James Almer írta:
> On 1/23/2017 11:25 AM, Steven Liu wrote:
>> 2017-01-23 22:14 GMT+08:00 James Almer <jamrial@gmail.com>:
>>
>>> On 1/23/2017 8:01 AM, Steven Liu wrote:
>>>> Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
>>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>>> ---
>>>>   libavformat/hlsenc.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>>> index 85d3955..0170b34 100644
>>>> --- a/libavformat/hlsenc.c
>>>> +++ b/libavformat/hlsenc.c
>>>> @@ -47,6 +47,7 @@ typedef enum {
>>>>
>>>>   #define KEYSIZE 16
>>>>   #define LINE_BUFFER_SIZE 1024
>>>> +#define HLS_MICROSECOND_UNIT   1000000
>>> You could instead use AV_TIME_BASE. It's more in line with the rest of the
>>> codebase.
>>>
>> No, the 1st version, Bodecs Bela has point the 1000000 is not AV_TIME_BASE
>> mean.
> I'm not sure i understand his reasoning, seeing that AV_TIME_BASE is
> defined as 1000000 in avutil.h and i don't see it changing anytime soon,
> but it's just a nit so not really important. This patch is good as is.
I used 1000000 because the duration is in seconds and the replacement 
should be in microseconds.
The filename will contain the duration in microseconds.
So this is a "seconds  to microseconds" conversion.  It would be a 
mistake to use AV_TIME_BASE here, because i think it is a coincindence 
that the 2 numbers is the same.
We want ot use microseconds in filenames not AV_TIME_BASE units.
Maybe a comment should be added to the code about it.

The reasoning behind to put duration in microseconds into filename 
instead of seconds was to avoid comma in filename.
>
>>>>   typedef struct HLSSegment {
>>>>       char filename[1024];
>>>> @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext
>>> *s, HLSContext *hls, double
>>>>                   return AVERROR(ENOMEM);
>>>>               }
>>>>               if (replace_int_data_in_filename(hls->avf->filename,
>>> sizeof(hls->avf->filename),
>>>> -                filename, 't',  (int64_t)round(1000000 * duration)) <
>>> 1) {
>>>> +                filename, 't',  (int64_t)round(duration *
>>> HLS_MICROSECOND_UNIT)) < 1) {
>>>>                   av_log(hls, AV_LOG_ERROR,
>>>>                          "Invalid second level segment filename template
>>> '%s', "
>>>>                           "you can try to remove
>>> second_level_segment_time flag\n",
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 Jan. 23, 2017, 3:18 p.m. UTC | #6 
On Mon, 23 Jan 2017 19:01:14 +0800
Steven Liu <lq@chinaffmpeg.org> wrote:

> Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/hlsenc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 85d3955..0170b34 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -47,6 +47,7 @@ typedef enum {
>  
>  #define KEYSIZE 16
>  #define LINE_BUFFER_SIZE 1024
> +#define HLS_MICROSECOND_UNIT   1000000
>  
>  typedef struct HLSSegment {
>      char filename[1024];
> @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext *s, HLSContext *hls, double
>                  return AVERROR(ENOMEM);
>              }
>              if (replace_int_data_in_filename(hls->avf->filename, sizeof(hls->avf->filename),
> -                filename, 't',  (int64_t)round(1000000 * duration)) < 1) {
> +                filename, 't',  (int64_t)round(duration * HLS_MICROSECOND_UNIT)) < 1) {
>                  av_log(hls, AV_LOG_ERROR,
>                         "Invalid second level segment filename template '%s', "
>                          "you can try to remove second_level_segment_time flag\n",

The commit message could be improved. It's barely English. (I'm not a
native speaker either, so I have no suggestion.)
Steven Liu Jan. 24, 2017, 4:32 a.m. UTC | #7 
2017-01-23 23:18 GMT+08:00 wm4 <nfxjfg@googlemail.com>:

> On Mon, 23 Jan 2017 19:01:14 +0800
> Steven Liu <lq@chinaffmpeg.org> wrote:
>
> > Reviewed-by: Bodecs Bela <bodecsb@vivanet.hu>
> > Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> > ---
> >  libavformat/hlsenc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> > index 85d3955..0170b34 100644
> > --- a/libavformat/hlsenc.c
> > +++ b/libavformat/hlsenc.c
> > @@ -47,6 +47,7 @@ typedef enum {
> >
> >  #define KEYSIZE 16
> >  #define LINE_BUFFER_SIZE 1024
> > +#define HLS_MICROSECOND_UNIT   1000000
> >
> >  typedef struct HLSSegment {
> >      char filename[1024];
> > @@ -501,7 +502,7 @@ static int hls_append_segment(struct AVFormatContext
> *s, HLSContext *hls, double
> >                  return AVERROR(ENOMEM);
> >              }
> >              if (replace_int_data_in_filename(hls->avf->filename,
> sizeof(hls->avf->filename),
> > -                filename, 't',  (int64_t)round(1000000 * duration)) <
> 1) {
> > +                filename, 't',  (int64_t)round(duration *
> HLS_MICROSECOND_UNIT)) < 1) {
> >                  av_log(hls, AV_LOG_ERROR,
> >                         "Invalid second level segment filename template
> '%s', "
> >                          "you can try to remove
> second_level_segment_time flag\n",
>
> The commit message could be improved. It's barely English. (I'm not a
> native speaker either, so I have no suggestion.)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

pushed
diff mbox

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 85d3955..0170b34 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -47,6 +47,7 @@  typedef enum {
 
 #define KEYSIZE 16
 #define LINE_BUFFER_SIZE 1024
+#define HLS_MICROSECOND_UNIT   1000000
 
 typedef struct HLSSegment {
     char filename[1024];
@@ -501,7 +502,7 @@  static int hls_append_segment(struct AVFormatContext *s, HLSContext *hls, double
                 return AVERROR(ENOMEM);
             }
             if (replace_int_data_in_filename(hls->avf->filename, sizeof(hls->avf->filename),
-                filename, 't',  (int64_t)round(1000000 * duration)) < 1) {
+                filename, 't',  (int64_t)round(duration * HLS_MICROSECOND_UNIT)) < 1) {
                 av_log(hls, AV_LOG_ERROR,
                        "Invalid second level segment filename template '%s', "
                         "you can try to remove second_level_segment_time flag\n",