Re: [FFmpeg-devel] [PATCH v2] avformat/srtenc: split write time into function for better readability

2020-04-29 Thread Nicolas George 
Limin Wang (12020-04-29):
> Sorry, the idea is coming from webvtt_write_time()

Well, I do not like Matthew Heaney's style. Everybody can use their
preferred style in their own files, of course.

Regards,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] avformat/srtenc: split write time into function for better readability

2020-04-29 Thread Limin Wang 
On Wed, Apr 29, 2020 at 04:38:42PM +0200, Nicolas George wrote:
> lance.lmw...@gmail.com (12020-04-29):
> > From: Limin Wang 
> > 
> > Signed-off-by: Limin Wang 
> > ---
> >  libavformat/srtenc.c | 24 ++--
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
> > index d811a4da0e..c53b227313 100644
> > --- a/libavformat/srtenc.c
> > +++ b/libavformat/srtenc.c
> > @@ -34,6 +34,20 @@ typedef struct SRTContext{
> >  unsigned index;
> >  } SRTContext;
> >  
> > +static void srt_write_time(AVIOContext *pb, int64_t millisec)
> > +{
> > +int64_t sec  = millisec / 1000;
> > +int64_t min  = sec / 60;
> > +int64_t hour = min / 60;
> > +
> > +millisec -= 1000 * sec;
> > +sec  -= 60 * min;
> > +min  -= 60 * hour;
> > +
> > +avio_printf(pb, "%02"PRId64":%02"PRId64":%02"PRId64",%03"PRId64"",
> > +hour, min, sec, millisec);
> > +}
> > +
> >  static int srt_write_header(AVFormatContext *avf)
> >  {
> >  SRTContext *srt = avf->priv_data;
> > @@ -85,12 +99,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >  return 0;
> >  }
> >  e = s + d;
> > -avio_printf(avf->pb, "%d\n%02d:%02d:%02d,%03d --> %02d:%02d:%02d,%03d",
> > -   srt->index,
> > -   (int)(s / 360),  (int)(s / 6) % 60,
> > -   (int)(s /1000) % 60, (int)(s %  1000),
> > -   (int)(e / 360),  (int)(e / 6) % 60,
> > -   (int)(e /1000) % 60, (int)(e %  1000));
> 
> I find the original code, with aligned divisions and explicit modulo
> more readable.
> 
> Your new code is fragile with regard to the order of operations, and it
> takes a little reflection to see it is correct. Not much, but more than
> (s / 6) % 60.

Sorry, the idea is coming from webvtt_write_time(), although I rewrite the code.
I think it's more readability to split the function with hour, minute, second, 
if you prefer to old code, then please ignore the patch.

> 
> Also, you are using PRId64 tu print numbers between 0 and 1000.
> 
> > +avio_printf(avf->pb, "%d\n", srt->index);
> > +srt_write_time(avf->pb, s);
> > +avio_printf(avf->pb, " --> ");
> > +srt_write_time(avf->pb, e);
> >  if (p)
> >  avio_printf(avf->pb, "  X1:%03d X2:%03d Y1:%03d Y2:%03d",
> >  x1, x2, y1, y2);
> 
> Regards,
> 
> -- 
>   Nicolas George



-- 
Thanks,
Limin Wang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] avformat/srtenc: split write time into function for better readability

2020-04-29 Thread Nicolas George 
lance.lmw...@gmail.com (12020-04-29):
> From: Limin Wang 
> 
> Signed-off-by: Limin Wang 
> ---
>  libavformat/srtenc.c | 24 ++--
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
> index d811a4da0e..c53b227313 100644
> --- a/libavformat/srtenc.c
> +++ b/libavformat/srtenc.c
> @@ -34,6 +34,20 @@ typedef struct SRTContext{
>  unsigned index;
>  } SRTContext;
>  
> +static void srt_write_time(AVIOContext *pb, int64_t millisec)
> +{
> +int64_t sec  = millisec / 1000;
> +int64_t min  = sec / 60;
> +int64_t hour = min / 60;
> +
> +millisec -= 1000 * sec;
> +sec  -= 60 * min;
> +min  -= 60 * hour;
> +
> +avio_printf(pb, "%02"PRId64":%02"PRId64":%02"PRId64",%03"PRId64"",
> +hour, min, sec, millisec);
> +}
> +
>  static int srt_write_header(AVFormatContext *avf)
>  {
>  SRTContext *srt = avf->priv_data;
> @@ -85,12 +99,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  return 0;
>  }
>  e = s + d;
> -avio_printf(avf->pb, "%d\n%02d:%02d:%02d,%03d --> %02d:%02d:%02d,%03d",
> -   srt->index,
> -   (int)(s / 360),  (int)(s / 6) % 60,
> -   (int)(s /1000) % 60, (int)(s %  1000),
> -   (int)(e / 360),  (int)(e / 6) % 60,
> -   (int)(e /1000) % 60, (int)(e %  1000));

I find the original code, with aligned divisions and explicit modulo
more readable.

Your new code is fragile with regard to the order of operations, and it
takes a little reflection to see it is correct. Not much, but more than
(s / 6) % 60.

Also, you are using PRId64 tu print numbers between 0 and 1000.

> +avio_printf(avf->pb, "%d\n", srt->index);
> +srt_write_time(avf->pb, s);
> +avio_printf(avf->pb, " --> ");
> +srt_write_time(avf->pb, e);
>  if (p)
>  avio_printf(avf->pb, "  X1:%03d X2:%03d Y1:%03d Y2:%03d",
>  x1, x2, y1, y2);

Regards,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".