| Message ID | 20200904231716.16182-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avcodec/jpeglsenc: Don't use put bits API for byte-aligned writes | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sat, Sep 05, 2020 at 01:17:09AM +0200, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Is it actually guaranteed that the allocated packet size is > sufficient? If it is, one could use unchecked writes throughout; if it > isn't, should I add a check to properly error out in case the buffer is > too small? (And what would the return value be in this case?) > > libavcodec/jpeglsenc.c | 80 +++++++++++++++++++++--------------- > libavcodec/mjpegenc.h | 6 --- > libavcodec/mjpegenc_common.c | 6 +++ > 3 files changed, 52 insertions(+), 40 deletions(-) > lgtm. Packet size may be smaller than needed, if there is no check it will fail. Usually it is allocated to max possible encoded packet size. Can not be sure, need to try encoding special pixels combinations that never happen in reality (almost). Ultimately some codecs check if encoded packet gonna exceed uncompressed frame than they just store uncompressed and properly mark bitstream. (Latest FFv1 encoder version, previous versions had this misfeature/bug)
On Sat, Sep 05, 2020 at 01:17:16AM +0200, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/jpegls.h | 4 ---- > 1 file changed, 4 deletions(-) LGTM thx [...]
Andreas Rheinhardt: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Is it actually guaranteed that the allocated packet size is > sufficient? If it is, one could use unchecked writes throughout; if it > isn't, should I add a check to properly error out in case the buffer is > too small? (And what would the return value be in this case?) > > libavcodec/jpeglsenc.c | 80 +++++++++++++++++++++--------------- > libavcodec/mjpegenc.h | 6 --- > libavcodec/mjpegenc_common.c | 6 +++ > 3 files changed, 52 insertions(+), 40 deletions(-) > > diff --git a/libavcodec/jpeglsenc.c b/libavcodec/jpeglsenc.c > index 5ecd430db7..feea6fdde7 100644 > --- a/libavcodec/jpeglsenc.c > +++ b/libavcodec/jpeglsenc.c > @@ -26,6 +26,7 @@ > */ > > #include "avcodec.h" > +#include "bytestream.h" > #include "get_bits.h" > #include "put_bits.h" > #include "golomb.h" > @@ -41,6 +42,18 @@ typedef struct JPEGLSContext { > int pred; > } JPEGLSContext; > > +static inline void put_markeru(PutByteContext *pb, enum JpegMarker code) > +{ > + bytestream2_put_byteu(pb, 0xff); > + bytestream2_put_byteu(pb, code); > +} > + > +static inline void put_marker(PutByteContext *pb, enum JpegMarker code) > +{ > + bytestream2_put_byte(pb, 0xff); > + bytestream2_put_byte(pb, code); > +} > + > /** > * Encode error from regular symbol > */ > @@ -230,7 +243,7 @@ static inline void ls_encode_line(JLSState *state, PutBitContext *pb, > } > } > > -static void ls_store_lse(JLSState *state, PutBitContext *pb) > +static void ls_store_lse(JLSState *state, PutByteContext *pb) > { > /* Test if we have default params and don't need to store LSE */ > JLSState state2 = { 0 }; > @@ -243,14 +256,14 @@ static void ls_store_lse(JLSState *state, PutBitContext *pb) > state->reset == state2.reset) > return; > /* store LSE type 1 */ > - put_marker(pb, LSE); > - put_bits(pb, 16, 13); > - put_bits(pb, 8, 1); > - put_bits(pb, 16, state->maxval); > - put_bits(pb, 16, state->T1); > - put_bits(pb, 16, state->T2); > - put_bits(pb, 16, state->T3); > - put_bits(pb, 16, state->reset); > + put_markeru(pb, LSE); > + bytestream2_put_be16u(pb, 13); > + bytestream2_put_byteu(pb, 1); > + bytestream2_put_be16u(pb, state->maxval); > + bytestream2_put_be16u(pb, state->T1); > + bytestream2_put_be16u(pb, state->T2); > + bytestream2_put_be16u(pb, state->T3); > + bytestream2_put_be16u(pb, state->reset); > } > > static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt, > @@ -258,7 +271,8 @@ static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt, > { > JPEGLSContext *ctx = avctx->priv_data; > const AVFrame *const p = pict; > - PutBitContext pb, pb2; > + PutByteContext pb; > + PutBitContext pb2; > GetBitContext gb; > uint8_t *buf2 = NULL; > uint8_t *zero = NULL; > @@ -289,33 +303,33 @@ FF_ENABLE_DEPRECATION_WARNINGS > if (!buf2) > goto memfail; > > - init_put_bits(&pb, pkt->data, pkt->size); > + bytestream2_init_writer(&pb, pkt->data, pkt->size); > init_put_bits(&pb2, buf2, pkt->size); > > /* write our own JPEG header, can't use mjpeg_picture_header */ > - put_marker(&pb, SOI); > - put_marker(&pb, SOF48); > - put_bits(&pb, 16, 8 + comps * 3); // header size depends on components > - put_bits(&pb, 8, (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8); // bpp > - put_bits(&pb, 16, avctx->height); > - put_bits(&pb, 16, avctx->width); > - put_bits(&pb, 8, comps); // components > + put_markeru(&pb, SOI); > + put_markeru(&pb, SOF48); > + bytestream2_put_be16u(&pb, 8 + comps * 3); // header size depends on components > + bytestream2_put_byteu(&pb, (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8); // bpp > + bytestream2_put_be16u(&pb, avctx->height); > + bytestream2_put_be16u(&pb, avctx->width); > + bytestream2_put_byteu(&pb, comps); // components > for (i = 1; i <= comps; i++) { > - put_bits(&pb, 8, i); // component ID > - put_bits(&pb, 8, 0x11); // subsampling: none > - put_bits(&pb, 8, 0); // Tiq, used by JPEG-LS ext > + bytestream2_put_byteu(&pb, i); // component ID > + bytestream2_put_byteu(&pb, 0x11); // subsampling: none > + bytestream2_put_byteu(&pb, 0); // Tiq, used by JPEG-LS ext > } > > - put_marker(&pb, SOS); > - put_bits(&pb, 16, 6 + comps * 2); > - put_bits(&pb, 8, comps); > + put_markeru(&pb, SOS); > + bytestream2_put_be16u(&pb, 6 + comps * 2); > + bytestream2_put_byteu(&pb, comps); > for (i = 1; i <= comps; i++) { > - put_bits(&pb, 8, i); // component ID > - put_bits(&pb, 8, 0); // mapping index: none > + bytestream2_put_byteu(&pb, i); // component ID > + bytestream2_put_byteu(&pb, 0); // mapping index: none > } > - put_bits(&pb, 8, ctx->pred); > - put_bits(&pb, 8, (comps > 1) ? 1 : 0); // interleaving: 0 - plane, 1 - line > - put_bits(&pb, 8, 0); // point transform: none > + bytestream2_put_byteu(&pb, ctx->pred); > + bytestream2_put_byteu(&pb, (comps > 1) ? 1 : 0); // interleaving: 0 - plane, 1 - line > + bytestream2_put_byteu(&pb, 0); // point transform: none > > state = av_mallocz(sizeof(JLSState)); > if (!state) > @@ -397,22 +411,20 @@ FF_ENABLE_DEPRECATION_WARNINGS > while (get_bits_count(&gb) < size) { > int v; > v = get_bits(&gb, 8); > - put_bits(&pb, 8, v); > + bytestream2_put_byte(&pb, v); > if (v == 0xFF) { > v = get_bits(&gb, 7); > - put_bits(&pb, 8, v); > + bytestream2_put_byte(&pb, v); > } > } > - avpriv_align_put_bits(&pb); > av_freep(&buf2); > > /* End of image */ > put_marker(&pb, EOI); > - flush_put_bits(&pb); > > emms_c(); > > - pkt->size = put_bits_count(&pb) >> 3; > + pkt->size = bytestream2_tell_p(&pb); > pkt->flags |= AV_PKT_FLAG_KEY; > *got_packet = 1; > return 0; > diff --git a/libavcodec/mjpegenc.h b/libavcodec/mjpegenc.h > index d7ddc35ef5..a4307dbf35 100644 > --- a/libavcodec/mjpegenc.h > +++ b/libavcodec/mjpegenc.h > @@ -98,12 +98,6 @@ enum HuffmanTableOption { > NB_HUFFMAN_TABLE_OPTION = 2 > }; > > -static inline void put_marker(PutBitContext *p, enum JpegMarker code) > -{ > - put_bits(p, 8, 0xff); > - put_bits(p, 8, code); > -} > - > int ff_mjpeg_encode_init(MpegEncContext *s); > void ff_mjpeg_encode_close(MpegEncContext *s); > void ff_mjpeg_encode_mb(MpegEncContext *s, int16_t block[12][64]); > diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c > index 3038ebde6e..fa965071a0 100644 > --- a/libavcodec/mjpegenc_common.c > +++ b/libavcodec/mjpegenc_common.c > @@ -62,6 +62,12 @@ av_cold void ff_init_uni_ac_vlc(const uint8_t huff_size_ac[256], uint8_t *uni_ac > } > } > > +static inline void put_marker(PutBitContext *p, enum JpegMarker code) > +{ > + put_bits(p, 8, 0xff); > + put_bits(p, 8, code); > +} > + > /* table_class: 0 = DC coef, 1 = AC coefs */ > static int put_huffman_table(PutBitContext *p, int table_class, int table_id, > const uint8_t *bits_table, const uint8_t *value_table) > I just noticed that I never applied this lgtm'ed patchset. Will rebase and apply. - Andreas
diff --git a/libavcodec/jpeglsenc.c b/libavcodec/jpeglsenc.c index 5ecd430db7..feea6fdde7 100644 --- a/libavcodec/jpeglsenc.c +++ b/libavcodec/jpeglsenc.c @@ -26,6 +26,7 @@ */ #include "avcodec.h" +#include "bytestream.h" #include "get_bits.h" #include "put_bits.h" #include "golomb.h" @@ -41,6 +42,18 @@ typedef struct JPEGLSContext { int pred; } JPEGLSContext; +static inline void put_markeru(PutByteContext *pb, enum JpegMarker code) +{ + bytestream2_put_byteu(pb, 0xff); + bytestream2_put_byteu(pb, code); +} + +static inline void put_marker(PutByteContext *pb, enum JpegMarker code) +{ + bytestream2_put_byte(pb, 0xff); + bytestream2_put_byte(pb, code); +} + /** * Encode error from regular symbol */ @@ -230,7 +243,7 @@ static inline void ls_encode_line(JLSState *state, PutBitContext *pb, } } -static void ls_store_lse(JLSState *state, PutBitContext *pb) +static void ls_store_lse(JLSState *state, PutByteContext *pb) { /* Test if we have default params and don't need to store LSE */ JLSState state2 = { 0 }; @@ -243,14 +256,14 @@ static void ls_store_lse(JLSState *state, PutBitContext *pb) state->reset == state2.reset) return; /* store LSE type 1 */ - put_marker(pb, LSE); - put_bits(pb, 16, 13); - put_bits(pb, 8, 1); - put_bits(pb, 16, state->maxval); - put_bits(pb, 16, state->T1); - put_bits(pb, 16, state->T2); - put_bits(pb, 16, state->T3); - put_bits(pb, 16, state->reset); + put_markeru(pb, LSE); + bytestream2_put_be16u(pb, 13); + bytestream2_put_byteu(pb, 1); + bytestream2_put_be16u(pb, state->maxval); + bytestream2_put_be16u(pb, state->T1); + bytestream2_put_be16u(pb, state->T2); + bytestream2_put_be16u(pb, state->T3); + bytestream2_put_be16u(pb, state->reset); } static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt, @@ -258,7 +271,8 @@ static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt, { JPEGLSContext *ctx = avctx->priv_data; const AVFrame *const p = pict; - PutBitContext pb, pb2; + PutByteContext pb; + PutBitContext pb2; GetBitContext gb; uint8_t *buf2 = NULL; uint8_t *zero = NULL; @@ -289,33 +303,33 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!buf2) goto memfail; - init_put_bits(&pb, pkt->data, pkt->size); + bytestream2_init_writer(&pb, pkt->data, pkt->size); init_put_bits(&pb2, buf2, pkt->size); /* write our own JPEG header, can't use mjpeg_picture_header */ - put_marker(&pb, SOI); - put_marker(&pb, SOF48); - put_bits(&pb, 16, 8 + comps * 3); // header size depends on components - put_bits(&pb, 8, (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8); // bpp - put_bits(&pb, 16, avctx->height); - put_bits(&pb, 16, avctx->width); - put_bits(&pb, 8, comps); // components + put_markeru(&pb, SOI); + put_markeru(&pb, SOF48); + bytestream2_put_be16u(&pb, 8 + comps * 3); // header size depends on components + bytestream2_put_byteu(&pb, (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8); // bpp + bytestream2_put_be16u(&pb, avctx->height); + bytestream2_put_be16u(&pb, avctx->width); + bytestream2_put_byteu(&pb, comps); // components for (i = 1; i <= comps; i++) { - put_bits(&pb, 8, i); // component ID - put_bits(&pb, 8, 0x11); // subsampling: none - put_bits(&pb, 8, 0); // Tiq, used by JPEG-LS ext + bytestream2_put_byteu(&pb, i); // component ID + bytestream2_put_byteu(&pb, 0x11); // subsampling: none + bytestream2_put_byteu(&pb, 0); // Tiq, used by JPEG-LS ext } - put_marker(&pb, SOS); - put_bits(&pb, 16, 6 + comps * 2); - put_bits(&pb, 8, comps); + put_markeru(&pb, SOS); + bytestream2_put_be16u(&pb, 6 + comps * 2); + bytestream2_put_byteu(&pb, comps); for (i = 1; i <= comps; i++) { - put_bits(&pb, 8, i); // component ID - put_bits(&pb, 8, 0); // mapping index: none + bytestream2_put_byteu(&pb, i); // component ID + bytestream2_put_byteu(&pb, 0); // mapping index: none } - put_bits(&pb, 8, ctx->pred); - put_bits(&pb, 8, (comps > 1) ? 1 : 0); // interleaving: 0 - plane, 1 - line - put_bits(&pb, 8, 0); // point transform: none + bytestream2_put_byteu(&pb, ctx->pred); + bytestream2_put_byteu(&pb, (comps > 1) ? 1 : 0); // interleaving: 0 - plane, 1 - line + bytestream2_put_byteu(&pb, 0); // point transform: none state = av_mallocz(sizeof(JLSState)); if (!state) @@ -397,22 +411,20 @@ FF_ENABLE_DEPRECATION_WARNINGS while (get_bits_count(&gb) < size) { int v; v = get_bits(&gb, 8); - put_bits(&pb, 8, v); + bytestream2_put_byte(&pb, v); if (v == 0xFF) { v = get_bits(&gb, 7); - put_bits(&pb, 8, v); + bytestream2_put_byte(&pb, v); } } - avpriv_align_put_bits(&pb); av_freep(&buf2); /* End of image */ put_marker(&pb, EOI); - flush_put_bits(&pb); emms_c(); - pkt->size = put_bits_count(&pb) >> 3; + pkt->size = bytestream2_tell_p(&pb); pkt->flags |= AV_PKT_FLAG_KEY; *got_packet = 1; return 0; diff --git a/libavcodec/mjpegenc.h b/libavcodec/mjpegenc.h index d7ddc35ef5..a4307dbf35 100644 --- a/libavcodec/mjpegenc.h +++ b/libavcodec/mjpegenc.h @@ -98,12 +98,6 @@ enum HuffmanTableOption { NB_HUFFMAN_TABLE_OPTION = 2 }; -static inline void put_marker(PutBitContext *p, enum JpegMarker code) -{ - put_bits(p, 8, 0xff); - put_bits(p, 8, code); -} - int ff_mjpeg_encode_init(MpegEncContext *s); void ff_mjpeg_encode_close(MpegEncContext *s); void ff_mjpeg_encode_mb(MpegEncContext *s, int16_t block[12][64]); diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c index 3038ebde6e..fa965071a0 100644 --- a/libavcodec/mjpegenc_common.c +++ b/libavcodec/mjpegenc_common.c @@ -62,6 +62,12 @@ av_cold void ff_init_uni_ac_vlc(const uint8_t huff_size_ac[256], uint8_t *uni_ac } } +static inline void put_marker(PutBitContext *p, enum JpegMarker code) +{ + put_bits(p, 8, 0xff); + put_bits(p, 8, code); +} + /* table_class: 0 = DC coef, 1 = AC coefs */ static int put_huffman_table(PutBitContext *p, int table_class, int table_id, const uint8_t *bits_table, const uint8_t *value_table)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Is it actually guaranteed that the allocated packet size is sufficient? If it is, one could use unchecked writes throughout; if it isn't, should I add a check to properly error out in case the buffer is too small? (And what would the return value be in this case?) libavcodec/jpeglsenc.c | 80 +++++++++++++++++++++--------------- libavcodec/mjpegenc.h | 6 --- libavcodec/mjpegenc_common.c | 6 +++ 3 files changed, 52 insertions(+), 40 deletions(-)