| Message ID | 20190804043231.74068-1-tellowkrinkle@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, 3 Aug 2019, Tellow Krinkle wrote: > Fixes #6390 > > Sample file for new test: https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass > > Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> > --- > libavformat/assenc.c | 4 +++- > tests/fate/subtitles.mak | 4 ++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavformat/assenc.c b/libavformat/assenc.c > index d50f18feb1..9b44b16597 100644 > --- a/libavformat/assenc.c > +++ b/libavformat/assenc.c > @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, int force) > ass->expected_readorder, dialogue->readorder); > ass->expected_readorder = dialogue->readorder; > } > - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); > + avio_write(s->pb, "Dialogue: ", 10); > + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); > + avio_write(s->pb, "\r\n", 2); IMHO avio_printf should be fixed instead to use an AVBPrint buffer, this should remove the 4k limit from it. Regards, Marton
On Sun, Aug 4, 2019 at 12:54 PM Marton Balint <cus@passwd.hu> wrote: > > > On Sat, 3 Aug 2019, Tellow Krinkle wrote: > > > Fixes #6390 > > > > Sample file for new test: > https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass > > > > Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> > > --- > > libavformat/assenc.c | 4 +++- > > tests/fate/subtitles.mak | 4 ++++ > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/assenc.c b/libavformat/assenc.c > > index d50f18feb1..9b44b16597 100644 > > --- a/libavformat/assenc.c > > +++ b/libavformat/assenc.c > > @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, int > force) > > ass->expected_readorder, dialogue->readorder); > > ass->expected_readorder = dialogue->readorder; > > } > > - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); > > + avio_write(s->pb, "Dialogue: ", 10); > > + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); > > + avio_write(s->pb, "\r\n", 2); > > IMHO avio_printf should be fixed instead to use an AVBPrint buffer, this > should remove the 4k limit from it. > That is unrelated issue. Using avio_printf in this specific case is overkill. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, 4 Aug 2019, Paul B Mahol wrote: > On Sun, Aug 4, 2019 at 12:54 PM Marton Balint <cus@passwd.hu> wrote: > >> >> >> On Sat, 3 Aug 2019, Tellow Krinkle wrote: >> >> > Fixes #6390 >> > >> > Sample file for new test: >> https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass >> > >> > Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> >> > --- >> > libavformat/assenc.c | 4 +++- >> > tests/fate/subtitles.mak | 4 ++++ >> > 2 files changed, 7 insertions(+), 1 deletion(-) >> > >> > diff --git a/libavformat/assenc.c b/libavformat/assenc.c >> > index d50f18feb1..9b44b16597 100644 >> > --- a/libavformat/assenc.c >> > +++ b/libavformat/assenc.c >> > @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, int >> force) >> > ass->expected_readorder, dialogue->readorder); >> > ass->expected_readorder = dialogue->readorder; >> > } >> > - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); >> > + avio_write(s->pb, "Dialogue: ", 10); >> > + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); >> > + avio_write(s->pb, "\r\n", 2); >> >> IMHO avio_printf should be fixed instead to use an AVBPrint buffer, this >> should remove the 4k limit from it. >> > > That is unrelated issue. Using avio_printf in this specific case is > overkill. Using 3 avio_writes instead is a useless micro optimalization which also reduces readblity, but feel free to apply if you feel stong about it. Regards, Marton
On Sun, Aug 4, 2019 at 2:58 PM Marton Balint <cus@passwd.hu> wrote: > > > On Sun, 4 Aug 2019, Paul B Mahol wrote: > > > On Sun, Aug 4, 2019 at 12:54 PM Marton Balint <cus@passwd.hu> wrote: > > > >> > >> > >> On Sat, 3 Aug 2019, Tellow Krinkle wrote: > >> > >> > Fixes #6390 > >> > > >> > Sample file for new test: > >> > https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass > >> > > >> > Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> > >> > --- > >> > libavformat/assenc.c | 4 +++- > >> > tests/fate/subtitles.mak | 4 ++++ > >> > 2 files changed, 7 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/libavformat/assenc.c b/libavformat/assenc.c > >> > index d50f18feb1..9b44b16597 100644 > >> > --- a/libavformat/assenc.c > >> > +++ b/libavformat/assenc.c > >> > @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, int > >> force) > >> > ass->expected_readorder, dialogue->readorder); > >> > ass->expected_readorder = dialogue->readorder; > >> > } > >> > - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); > >> > + avio_write(s->pb, "Dialogue: ", 10); > >> > + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); > >> > + avio_write(s->pb, "\r\n", 2); > >> > >> IMHO avio_printf should be fixed instead to use an AVBPrint buffer, this > >> should remove the 4k limit from it. > >> > > > > That is unrelated issue. Using avio_printf in this specific case is > > overkill. > > Using 3 avio_writes instead is a useless micro optimalization which also > reduces readblity, but feel free to apply if you feel stong about it. > I doubt so, make actual benchmark to prove your claims. Allocation + memcpy is slower than direct writing. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, 4 Aug 2019, Paul B Mahol wrote: > On Sun, Aug 4, 2019 at 2:58 PM Marton Balint <cus@passwd.hu> wrote: > >> >> >> On Sun, 4 Aug 2019, Paul B Mahol wrote: >> >> > On Sun, Aug 4, 2019 at 12:54 PM Marton Balint <cus@passwd.hu> wrote: >> > >> >> >> >> >> >> On Sat, 3 Aug 2019, Tellow Krinkle wrote: >> >> >> >> > Fixes #6390 >> >> > >> >> > Sample file for new test: >> >> >> https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass >> >> > >> >> > Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> >> >> > --- >> >> > libavformat/assenc.c | 4 +++- >> >> > tests/fate/subtitles.mak | 4 ++++ >> >> > 2 files changed, 7 insertions(+), 1 deletion(-) >> >> > >> >> > diff --git a/libavformat/assenc.c b/libavformat/assenc.c >> >> > index d50f18feb1..9b44b16597 100644 >> >> > --- a/libavformat/assenc.c >> >> > +++ b/libavformat/assenc.c >> >> > @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, int >> >> force) >> >> > ass->expected_readorder, dialogue->readorder); >> >> > ass->expected_readorder = dialogue->readorder; >> >> > } >> >> > - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); >> >> > + avio_write(s->pb, "Dialogue: ", 10); >> >> > + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); >> >> > + avio_write(s->pb, "\r\n", 2); >> >> >> >> IMHO avio_printf should be fixed instead to use an AVBPrint buffer, this >> >> should remove the 4k limit from it. >> >> >> > >> > That is unrelated issue. Using avio_printf in this specific case is >> > overkill. >> >> Using 3 avio_writes instead is a useless micro optimalization which also >> reduces readblity, but feel free to apply if you feel stong about it. >> > > I doubt so, make actual benchmark to prove your claims. > Allocation + memcpy is slower than direct writing. And considering the size of ASS files (a few hundred KB) it will not be measureable overall, only if you benchmark the actual function. If you are really concerened, we could add a function like this: int avio_print_n_strings(AVIOContext *s, int nb_strings, ...) { va_list ap; int ret = 0; va_start(ap, nb_strings); for (int i = 0; i < nb_strings; i++) { const char *str = va_arg(ap, const char *); int len = strlen(str); avio_write(s, (const unsigned char *)str, len); ret += len; } va_end(ap); return ret; } /** * Write nb_strings number of strings (const char *) to the context. */ #define avio_print(s, ...) avio_print_n_strings(s, sizeof((const char*[]){__VA_ARGS__}) / sizeof(const char*), __VA_ARGS__); So from ASSenc you could use: avio_print(s->pb, "Dialogue: ", dialogue->line, "\r\n"); Regards, Marton
Please send such patch. On Sun, Aug 4, 2019 at 5:41 PM Marton Balint <cus@passwd.hu> wrote: > > > On Sun, 4 Aug 2019, Paul B Mahol wrote: > > > On Sun, Aug 4, 2019 at 2:58 PM Marton Balint <cus@passwd.hu> wrote: > > > >> > >> > >> On Sun, 4 Aug 2019, Paul B Mahol wrote: > >> > >> > On Sun, Aug 4, 2019 at 12:54 PM Marton Balint <cus@passwd.hu> wrote: > >> > > >> >> > >> >> > >> >> On Sat, 3 Aug 2019, Tellow Krinkle wrote: > >> >> > >> >> > Fixes #6390 > >> >> > > >> >> > Sample file for new test: > >> >> > >> > https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass > >> >> > > >> >> > Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> > >> >> > --- > >> >> > libavformat/assenc.c | 4 +++- > >> >> > tests/fate/subtitles.mak | 4 ++++ > >> >> > 2 files changed, 7 insertions(+), 1 deletion(-) > >> >> > > >> >> > diff --git a/libavformat/assenc.c b/libavformat/assenc.c > >> >> > index d50f18feb1..9b44b16597 100644 > >> >> > --- a/libavformat/assenc.c > >> >> > +++ b/libavformat/assenc.c > >> >> > @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, > int > >> >> force) > >> >> > ass->expected_readorder, dialogue->readorder); > >> >> > ass->expected_readorder = dialogue->readorder; > >> >> > } > >> >> > - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); > >> >> > + avio_write(s->pb, "Dialogue: ", 10); > >> >> > + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); > >> >> > + avio_write(s->pb, "\r\n", 2); > >> >> > >> >> IMHO avio_printf should be fixed instead to use an AVBPrint buffer, > this > >> >> should remove the 4k limit from it. > >> >> > >> > > >> > That is unrelated issue. Using avio_printf in this specific case is > >> > overkill. > >> > >> Using 3 avio_writes instead is a useless micro optimalization which also > >> reduces readblity, but feel free to apply if you feel stong about it. > >> > > > > I doubt so, make actual benchmark to prove your claims. > > Allocation + memcpy is slower than direct writing. > > And considering the size of ASS files (a few hundred KB) it will not be > measureable overall, only if you benchmark the actual function. > > If you are really concerened, we could add a function like this: > > int avio_print_n_strings(AVIOContext *s, int nb_strings, ...) > { > va_list ap; > int ret = 0; > > va_start(ap, nb_strings); > for (int i = 0; i < nb_strings; i++) { > const char *str = va_arg(ap, const char *); > int len = strlen(str); > avio_write(s, (const unsigned char *)str, len); > ret += len; > } > va_end(ap); > > return ret; > } > > /** > * Write nb_strings number of strings (const char *) to the context. > */ > #define avio_print(s, ...) avio_print_n_strings(s, sizeof((const > char*[]){__VA_ARGS__}) / sizeof(const char*), __VA_ARGS__); > > So from ASSenc you could use: > > avio_print(s->pb, "Dialogue: ", dialogue->line, "\r\n"); > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavformat/assenc.c b/libavformat/assenc.c index d50f18feb1..9b44b16597 100644 --- a/libavformat/assenc.c +++ b/libavformat/assenc.c @@ -95,7 +95,9 @@ static void purge_dialogues(AVFormatContext *s, int force) ass->expected_readorder, dialogue->readorder); ass->expected_readorder = dialogue->readorder; } - avio_printf(s->pb, "Dialogue: %s\r\n", dialogue->line); + avio_write(s->pb, "Dialogue: ", 10); + avio_write(s->pb, dialogue->line, strlen(dialogue->line)); + avio_write(s->pb, "\r\n", 2); if (dialogue == ass->last_added_dialogue) ass->last_added_dialogue = next; av_freep(&dialogue->line); diff --git a/tests/fate/subtitles.mak b/tests/fate/subtitles.mak index 0042902161..a4e0fc7432 100644 --- a/tests/fate/subtitles.mak +++ b/tests/fate/subtitles.mak @@ -13,6 +13,10 @@ fate-sub-cc-scte20: CMD = fmtstdout ass -f lavfi -i "movie=$(TARGET_SAMPLES)/sub FATE_SUBTITLES_ASS-$(call DEMDEC, ASS, ASS) += fate-sub-ass-to-ass-transcode fate-sub-ass-to-ass-transcode: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/1ededcbd7b.ass +FATE_SUBTITLES_ASS-$(call DEMDEC, ASS, ASS) += fate-sub-ass-long-line +fate-sub-ass-long-line: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/longline.ass +fate-sub-ass-long-line: REF = $(TARGET_SAMPLES)/sub/longline.ass + FATE_SUBTITLES_ASS-$(CONFIG_ASS_DEMUXER) += fate-sub-ssa-to-ass-remux fate-sub-ssa-to-ass-remux: CMD = fmtstdout ass -i $(TARGET_SAMPLES)/sub/a9-misc.ssa -c copy
Fixes #6390 Sample file for new test: https://gist.githubusercontent.com/tellowkrinkle/d6a6e328f892dbbacc000ad9c3890644/raw/4f68e56b1f0fab594aae040723722af4f5161a02/longline.ass Signed-off-by: Tellow Krinkle <tellowkrinkle@gmail.com> --- libavformat/assenc.c | 4 +++- tests/fate/subtitles.mak | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-)