Re: [FFmpeg-devel] [PATCH v2] avformat/srtenc: split write time into function for better readability
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
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
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".
[FFmpeg-devel] [PATCH v2] avformat/srtenc: split write time into function for better readability
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)); +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); -- 2.21.0 ___ 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".