| Message ID | AS8P250MB0744F752DB1C2683934C27368F212@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 878f8cabe8d2be6f94875da3857a5271bb56c928 |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/xbmenc: Avoid snprintf() for data->hex conversion | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Andreas Rheinhardt: > Use a small LUT instead. Improves performance. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/xbmenc.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c > index cd8b73afa3..5231d4691d 100644 > --- a/libavcodec/xbmenc.c > +++ b/libavcodec/xbmenc.c > @@ -20,11 +20,9 @@ > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > */ > > -#include "libavutil/reverse.h" > #include "avcodec.h" > #include "codec_internal.h" > #include "encode.h" > -#include "mathops.h" > > #define ANSI_MIN_READLINE 509 > > @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext *avctx, AVPacket *pkt, > buf += snprintf(buf, 39, "static unsigned char image_bits[] = {\n"); > for (i = 0, l = lineout; i < avctx->height; i++) { > for (j = 0; j < linesize; j++) { > - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); > + // 0..15 bitreversed as chars > + static const char lut[] = { > + '0', '8', '4', 'C', '2', 'A', '6', 'E', > + '1', '9', '5', 'D', '3', 'B', '7', 'F' > + }; > + buf[0] = ' '; > + buf[1] = '0'; > + buf[2] = 'x'; > + buf[3] = lut[*ptr & 0xF]; > + buf[4] = lut[*ptr >> 4]; > + buf += 5; > + ptr++; > if (--commas <= 0) { > - buf += snprintf(buf, 2, "\n"); > + *buf++ = '\n'; > break; > } > - buf += snprintf(buf, 2, ","); > + *buf++ = ','; > if (--l <= 0) { > - buf += snprintf(buf, 2, "\n"); > + *buf++ = '\n'; > l = lineout; > } > } Will apply this patch tomorrow unless there are objections. - Andreas
On Fri, 8 Mar 2024, Andreas Rheinhardt wrote: > Andreas Rheinhardt: >> Use a small LUT instead. Improves performance. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/xbmenc.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c >> index cd8b73afa3..5231d4691d 100644 >> --- a/libavcodec/xbmenc.c >> +++ b/libavcodec/xbmenc.c >> @@ -20,11 +20,9 @@ >> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA >> */ >> >> -#include "libavutil/reverse.h" >> #include "avcodec.h" >> #include "codec_internal.h" >> #include "encode.h" >> -#include "mathops.h" >> >> #define ANSI_MIN_READLINE 509 >> >> @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext *avctx, AVPacket *pkt, >> buf += snprintf(buf, 39, "static unsigned char image_bits[] = {\n"); >> for (i = 0, l = lineout; i < avctx->height; i++) { >> for (j = 0; j < linesize; j++) { >> - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); >> + // 0..15 bitreversed as chars >> + static const char lut[] = { >> + '0', '8', '4', 'C', '2', 'A', '6', 'E', >> + '1', '9', '5', 'D', '3', 'B', '7', 'F' >> + }; >> + buf[0] = ' '; >> + buf[1] = '0'; >> + buf[2] = 'x'; >> + buf[3] = lut[*ptr & 0xF]; >> + buf[4] = lut[*ptr >> 4]; Maybe you could use *buf++ = ... here as well, to avoid the next line. But fine either way I guess. Thanks, Marton >> + buf += 5; >> + ptr++; >> if (--commas <= 0) { >> - buf += snprintf(buf, 2, "\n"); >> + *buf++ = '\n'; >> break; >> } >> - buf += snprintf(buf, 2, ","); >> + *buf++ = ','; >> if (--l <= 0) { >> - buf += snprintf(buf, 2, "\n"); >> + *buf++ = '\n'; >> l = lineout; >> } >> } > > Will apply this patch tomorrow unless there are objections. > > - Andreas > > _______________________________________________ > 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". >
Marton Balint: > > > On Fri, 8 Mar 2024, Andreas Rheinhardt wrote: > >> Andreas Rheinhardt: >>> Use a small LUT instead. Improves performance. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavcodec/xbmenc.c | 21 +++++++++++++++------ >>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c >>> index cd8b73afa3..5231d4691d 100644 >>> --- a/libavcodec/xbmenc.c >>> +++ b/libavcodec/xbmenc.c >>> @@ -20,11 +20,9 @@ >>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>> 02110-1301 USA >>> */ >>> >>> -#include "libavutil/reverse.h" >>> #include "avcodec.h" >>> #include "codec_internal.h" >>> #include "encode.h" >>> -#include "mathops.h" >>> >>> #define ANSI_MIN_READLINE 509 >>> >>> @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext >>> *avctx, AVPacket *pkt, >>> buf += snprintf(buf, 39, "static unsigned char image_bits[] = >>> {\n"); >>> for (i = 0, l = lineout; i < avctx->height; i++) { >>> for (j = 0; j < linesize; j++) { >>> - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); >>> + // 0..15 bitreversed as chars >>> + static const char lut[] = { >>> + '0', '8', '4', 'C', '2', 'A', '6', 'E', >>> + '1', '9', '5', 'D', '3', 'B', '7', 'F' >>> + }; >>> + buf[0] = ' '; >>> + buf[1] = '0'; >>> + buf[2] = 'x'; >>> + buf[3] = lut[*ptr & 0xF]; >>> + buf[4] = lut[*ptr >> 4]; > > Maybe you could use *buf++ = ... here as well, to avoid the next line. > But fine either way I guess. > You mean *ptr++ to avoid the line after the next line? That would make the two lut accesses unsymmetric. And actually I prefer that both pointers are incremented side-by-side. >>> + buf += 5; >>> + ptr++;
On Fri, 8 Mar 2024, Andreas Rheinhardt wrote: > Marton Balint: >> >> >> On Fri, 8 Mar 2024, Andreas Rheinhardt wrote: >> >>> Andreas Rheinhardt: >>>> Use a small LUT instead. Improves performance. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavcodec/xbmenc.c | 21 +++++++++++++++------ >>>> 1 file changed, 15 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c >>>> index cd8b73afa3..5231d4691d 100644 >>>> --- a/libavcodec/xbmenc.c >>>> +++ b/libavcodec/xbmenc.c >>>> @@ -20,11 +20,9 @@ >>>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>>> 02110-1301 USA >>>> */ >>>> >>>> -#include "libavutil/reverse.h" >>>> #include "avcodec.h" >>>> #include "codec_internal.h" >>>> #include "encode.h" >>>> -#include "mathops.h" >>>> >>>> #define ANSI_MIN_READLINE 509 >>>> >>>> @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext >>>> *avctx, AVPacket *pkt, >>>> buf += snprintf(buf, 39, "static unsigned char image_bits[] = >>>> {\n"); >>>> for (i = 0, l = lineout; i < avctx->height; i++) { >>>> for (j = 0; j < linesize; j++) { >>>> - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); >>>> + // 0..15 bitreversed as chars >>>> + static const char lut[] = { >>>> + '0', '8', '4', 'C', '2', 'A', '6', 'E', >>>> + '1', '9', '5', 'D', '3', 'B', '7', 'F' >>>> + }; >>>> + buf[0] = ' '; >>>> + buf[1] = '0'; >>>> + buf[2] = 'x'; >>>> + buf[3] = lut[*ptr & 0xF]; >>>> + buf[4] = lut[*ptr >> 4]; >> >> Maybe you could use *buf++ = ... here as well, to avoid the next line. >> But fine either way I guess. >> > > You mean *ptr++ to avoid the line after the next line? That would make > the two lut accesses unsymmetric. And actually I prefer that both > pointers are incremented side-by-side. > I meant this: diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c index 5231d4691d..20f8951f93 100644 --- a/libavcodec/xbmenc.c +++ b/libavcodec/xbmenc.c @@ -60,12 +60,11 @@ static int xbm_encode_frame(AVCodecContext *avctx, AVPacket *pkt, '0', '8', '4', 'C', '2', 'A', '6', 'E', '1', '9', '5', 'D', '3', 'B', '7', 'F' }; - buf[0] = ' '; - buf[1] = '0'; - buf[2] = 'x'; - buf[3] = lut[*ptr & 0xF]; - buf[4] = lut[*ptr >> 4]; - buf += 5; + *buf++ = ' '; + *buf++ = '0'; + *buf++ = 'x'; + *buf++ = lut[*ptr & 0xF]; + *buf++ = lut[*ptr >> 4]; ptr++; if (--commas <= 0) { *buf++ = '\n'; Regards, Marton
Marton Balint: > > > On Fri, 8 Mar 2024, Andreas Rheinhardt wrote: > >> Marton Balint: >>> >>> >>> On Fri, 8 Mar 2024, Andreas Rheinhardt wrote: >>> >>>> Andreas Rheinhardt: >>>>> Use a small LUT instead. Improves performance. >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>> --- >>>>> libavcodec/xbmenc.c | 21 +++++++++++++++------ >>>>> 1 file changed, 15 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c >>>>> index cd8b73afa3..5231d4691d 100644 >>>>> --- a/libavcodec/xbmenc.c >>>>> +++ b/libavcodec/xbmenc.c >>>>> @@ -20,11 +20,9 @@ >>>>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>>>> 02110-1301 USA >>>>> */ >>>>> >>>>> -#include "libavutil/reverse.h" >>>>> #include "avcodec.h" >>>>> #include "codec_internal.h" >>>>> #include "encode.h" >>>>> -#include "mathops.h" >>>>> >>>>> #define ANSI_MIN_READLINE 509 >>>>> >>>>> @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext >>>>> *avctx, AVPacket *pkt, >>>>> buf += snprintf(buf, 39, "static unsigned char image_bits[] = >>>>> {\n"); >>>>> for (i = 0, l = lineout; i < avctx->height; i++) { >>>>> for (j = 0; j < linesize; j++) { >>>>> - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); >>>>> + // 0..15 bitreversed as chars >>>>> + static const char lut[] = { >>>>> + '0', '8', '4', 'C', '2', 'A', '6', 'E', >>>>> + '1', '9', '5', 'D', '3', 'B', '7', 'F' >>>>> + }; >>>>> + buf[0] = ' '; >>>>> + buf[1] = '0'; >>>>> + buf[2] = 'x'; >>>>> + buf[3] = lut[*ptr & 0xF]; >>>>> + buf[4] = lut[*ptr >> 4]; >>> >>> Maybe you could use *buf++ = ... here as well, to avoid the next line. >>> But fine either way I guess. >>> >> >> You mean *ptr++ to avoid the line after the next line? That would make >> the two lut accesses unsymmetric. And actually I prefer that both >> pointers are incremented side-by-side. >> > > I meant this: > > diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c > index 5231d4691d..20f8951f93 100644 > --- a/libavcodec/xbmenc.c > +++ b/libavcodec/xbmenc.c > @@ -60,12 +60,11 @@ static int xbm_encode_frame(AVCodecContext *avctx, > AVPacket *pkt, > '0', '8', '4', 'C', '2', 'A', '6', 'E', > '1', '9', '5', 'D', '3', 'B', '7', 'F' > }; > - buf[0] = ' '; > - buf[1] = '0'; > - buf[2] = 'x'; > - buf[3] = lut[*ptr & 0xF]; > - buf[4] = lut[*ptr >> 4]; > - buf += 5; > + *buf++ = ' '; > + *buf++ = '0'; > + *buf++ = 'x'; > + *buf++ = lut[*ptr & 0xF]; > + *buf++ = lut[*ptr >> 4]; > ptr++; > if (--commas <= 0) { > *buf++ = '\n'; > > Ok, I misunderstood. But I still prefer my version. - Andreas
Andreas Rheinhardt: > Use a small LUT instead. Improves performance. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/xbmenc.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c > index cd8b73afa3..5231d4691d 100644 > --- a/libavcodec/xbmenc.c > +++ b/libavcodec/xbmenc.c > @@ -20,11 +20,9 @@ > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > */ > > -#include "libavutil/reverse.h" > #include "avcodec.h" > #include "codec_internal.h" > #include "encode.h" > -#include "mathops.h" > > #define ANSI_MIN_READLINE 509 > > @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext *avctx, AVPacket *pkt, > buf += snprintf(buf, 39, "static unsigned char image_bits[] = {\n"); > for (i = 0, l = lineout; i < avctx->height; i++) { > for (j = 0; j < linesize; j++) { > - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); > + // 0..15 bitreversed as chars > + static const char lut[] = { > + '0', '8', '4', 'C', '2', 'A', '6', 'E', > + '1', '9', '5', 'D', '3', 'B', '7', 'F' > + }; > + buf[0] = ' '; > + buf[1] = '0'; > + buf[2] = 'x'; > + buf[3] = lut[*ptr & 0xF]; > + buf[4] = lut[*ptr >> 4]; > + buf += 5; > + ptr++; > if (--commas <= 0) { > - buf += snprintf(buf, 2, "\n"); > + *buf++ = '\n'; > break; > } > - buf += snprintf(buf, 2, ","); > + *buf++ = ','; > if (--l <= 0) { > - buf += snprintf(buf, 2, "\n"); > + *buf++ = '\n'; > l = lineout; > } > } Will apply. - Andreas
diff --git a/libavcodec/xbmenc.c b/libavcodec/xbmenc.c index cd8b73afa3..5231d4691d 100644 --- a/libavcodec/xbmenc.c +++ b/libavcodec/xbmenc.c @@ -20,11 +20,9 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -#include "libavutil/reverse.h" #include "avcodec.h" #include "codec_internal.h" #include "encode.h" -#include "mathops.h" #define ANSI_MIN_READLINE 509 @@ -57,14 +55,25 @@ static int xbm_encode_frame(AVCodecContext *avctx, AVPacket *pkt, buf += snprintf(buf, 39, "static unsigned char image_bits[] = {\n"); for (i = 0, l = lineout; i < avctx->height; i++) { for (j = 0; j < linesize; j++) { - buf += snprintf(buf, 6, " 0x%02X", ff_reverse[*ptr++]); + // 0..15 bitreversed as chars + static const char lut[] = { + '0', '8', '4', 'C', '2', 'A', '6', 'E', + '1', '9', '5', 'D', '3', 'B', '7', 'F' + }; + buf[0] = ' '; + buf[1] = '0'; + buf[2] = 'x'; + buf[3] = lut[*ptr & 0xF]; + buf[4] = lut[*ptr >> 4]; + buf += 5; + ptr++; if (--commas <= 0) { - buf += snprintf(buf, 2, "\n"); + *buf++ = '\n'; break; } - buf += snprintf(buf, 2, ","); + *buf++ = ','; if (--l <= 0) { - buf += snprintf(buf, 2, "\n"); + *buf++ = '\n'; l = lineout; } }
Use a small LUT instead. Improves performance. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/xbmenc.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)