diff mbox series

[FFmpeg-devel,1/8] avcodec/jpeglsenc: Don't use put bits API for byte-aligned writes

Message ID 20200904231716.16182-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/8] avcodec/jpeglsenc: Don't use put bits API for byte-aligned writes
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 4, 2020, 11:17 p.m. UTC
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(-)

Comments

Paul B Mahol Sept. 6, 2020, 5:09 p.m. UTC | #1
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)
Michael Niedermayer Sept. 6, 2020, 9:07 p.m. UTC | #2
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

[...]
diff mbox series

Patch

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)