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

[Nicolas George]

lance.lmwang at gmail.com (12020-04-29):
> From: Limin Wang <lance.lmwang at gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> ---
>  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 / 3600000),      (int)(s / 60000) % 60,
> -                   (int)(s /    1000) % 60, (int)(s %  1000),
> -                   (int)(e / 3600000),      (int)(e / 60000) % 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 / 60000) % 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200429/8fab5df4/attachment.sig>