[FFmpeg-devel] [PATCH 1/6] cbs_apv: Always restore tracing state on split fragment error

[FFmpeg-devel] [PATCH 1/6] cbs_apv: Always restore tracing state on split fragment error

[Mark Thompson]

Fixes CID 1646769.
---
 libavcodec/cbs_apv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/cbs_apv.c b/libavcodec/cbs_apv.c
index e7dd04825c..ebf57d3bbb 100644
--- a/libavcodec/cbs_apv.c
+++ b/libavcodec/cbs_apv.c
@@ -230,14 +230,14 @@ static int cbs_apv_split_fragment(CodedBitstreamContext *ctx,
 
         err = cbs_apv_read_pbu_header(ctx, &gbc, &pbu_header);
         if (err < 0)
-            return err;
+            goto fail;
 
         // Could select/skip frames based on type/group_id here.
 
         err = ff_cbs_append_unit_data(frag, pbu_header.pbu_type,
                                       data, pbu_size, frag->data_ref);
         if (err < 0)
-            return err;
+            goto fail;
 
         data += pbu_size;
         size -= pbu_size;
-- 
2.47.2

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/6] apv_decode: Fix memory leak on decode error

[Mark Thompson]

---
 libavcodec/apv_decode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavcodec/apv_decode.c b/libavcodec/apv_decode.c
index b1e1db7d64..2a59c9b25d 100644
--- a/libavcodec/apv_decode.c
+++ b/libavcodec/apv_decode.c
@@ -377,7 +377,7 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
         case APV_PBU_PRIMARY_FRAME:
             err = apv_decode(avctx, frame, pbu->content);
             if (err < 0)
-                return err;
+                goto fail;
             *got_frame = 1;
             break;
         case APV_PBU_METADATA:
@@ -412,8 +412,11 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
     }
 
     ff_cbs_fragment_reset(au);
-
     return packet->size;
+
+fail:
+    ff_cbs_fragment_reset(au);
+    return err;
 }
 
 const FFCodec ff_apv_decoder = {
-- 
2.47.2

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/6] apv_decode: Improve reporting of decode errors

[Mark Thompson]

Halt tile component decoding at the first entropy error (this will be a
desync and is not recoverable).  If any tile components contain errors
then discard the frame unless the output-corrupt flag is set.

Also fixes CID 1646764, which is the error case where the tile component
is too large for get_bits to handle.
---
 libavcodec/apv_decode.c | 52 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 9 deletions(-)

diff --git a/libavcodec/apv_decode.c b/libavcodec/apv_decode.c
index 2a59c9b25d..7ee39e578a 100644
--- a/libavcodec/apv_decode.c
+++ b/libavcodec/apv_decode.c
@@ -16,6 +16,8 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include <stdatomic.h>
+
 #include "libavutil/mastering_display_metadata.h"
 #include "libavutil/mem_internal.h"
 #include "libavutil/pixdesc.h"
@@ -42,6 +44,7 @@ typedef struct APVDecodeContext {
     APVVLCLUT decode_lut;
 
     AVFrame *output_frame;
+    atomic_int tile_errors;
 
     uint8_t warned_additional_frames;
     uint8_t warned_unknown_pbu_types;
@@ -121,6 +124,8 @@ static av_cold int apv_decode_init(AVCodecContext *avctx)
 
     ff_apv_dsp_init(&apv->dsp);
 
+    atomic_init(&apv->tile_errors, 0);
+
     return 0;
 }
 
@@ -150,7 +155,7 @@ static int apv_decode_block(AVCodecContext *avctx,
 
     err = ff_apv_entropy_decode_block(coeff, gbc, entropy_state);
     if (err < 0)
-        return 0;
+        return err;
 
     apv->dsp.decode_transquant(output, pitch,
                                coeff, qmatrix,
@@ -207,8 +212,12 @@ static int apv_decode_tile_component(AVCodecContext *avctx, void *data,
         .prev_1st_ac_level = 0,
     };
 
-    init_get_bits8(&gbc, tile->tile_data[comp_index],
-                   tile->tile_header.tile_data_size[comp_index]);
+    int err;
+
+    err = init_get_bits8(&gbc, tile->tile_data[comp_index],
+                         tile->tile_header.tile_data_size[comp_index]);
+    if (err < 0)
+        goto fail;
 
     // Combine the bitstream quantisation matrix with the qp scaling
     // in advance.  (Including qp_shift as well would overflow 16 bits.)
@@ -243,12 +252,17 @@ static int apv_decode_tile_component(AVCodecContext *avctx, void *data,
                     uint8_t  *block_start = apv->output_frame->data[comp_index] +
                                             frame_y * frame_pitch + 2 * frame_x;
 
-                    apv_decode_block(avctx,
-                                     block_start, frame_pitch,
-                                     &gbc, &entropy_state,
-                                     bit_depth,
-                                     qp_shift,
-                                     qmatrix_scaled);
+                    err = apv_decode_block(avctx,
+                                           block_start, frame_pitch,
+                                           &gbc, &entropy_state,
+                                           bit_depth,
+                                           qp_shift,
+                                           qmatrix_scaled);
+                    if (err < 0) {
+                        // Error in block decode means entropy desync,
+                        // so this is not recoverable.
+                        goto fail;
+                    }
                 }
             }
         }
@@ -260,6 +274,13 @@ static int apv_decode_tile_component(AVCodecContext *avctx, void *data,
            tile_start_x, tile_start_y);
 
     return 0;
+
+fail:
+    av_log(avctx, AV_LOG_VERBOSE,
+           "Decode error in tile %d component %d.\n",
+           tile_index, comp_index);
+    atomic_fetch_add(&apv->tile_errors, 1);
+    return err;
 }
 
 static int apv_decode(AVCodecContext *avctx, AVFrame *output,
@@ -281,6 +302,7 @@ static int apv_decode(AVCodecContext *avctx, AVFrame *output,
         return err;
 
     apv->output_frame = output;
+    atomic_store(&apv->tile_errors, 0);
 
     // Each component within a tile is independent of every other,
     // so we can decode all in parallel.
@@ -289,6 +311,18 @@ static int apv_decode(AVCodecContext *avctx, AVFrame *output,
     avctx->execute2(avctx, apv_decode_tile_component,
                     input, NULL, job_count);
 
+    err = atomic_load(&apv->tile_errors);
+    if (err > 0) {
+        av_log(avctx, AV_LOG_ERROR,
+               "Decode errors in %d tile components.\n", err);
+        if (avctx->flags & AV_CODEC_FLAG_OUTPUT_CORRUPT) {
+            // Output the frame anyway.
+            output->flags |= AV_FRAME_FLAG_CORRUPT;
+        } else {
+            return AVERROR_INVALIDDATA;
+        }
+    }
+
     return 0;
 }
 
-- 
2.47.2

_______________________________________________
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".

[FFmpeg-devel] [PATCH 4/6] cbs_apv: Better constrain tile_width/height_in_mbs

[Mark Thompson]

The maximum number of tile columns/rows adds an extra constraint on
the minimum tile width/height for large frames (over 5120 width or
2560 height).
---
 libavcodec/apv_decode.c              |  2 +-
 libavcodec/cbs_apv_syntax_template.c | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/libavcodec/apv_decode.c b/libavcodec/apv_decode.c
index 7ee39e578a..7761a155ae 100644
--- a/libavcodec/apv_decode.c
+++ b/libavcodec/apv_decode.c
@@ -401,7 +401,7 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
     err = ff_cbs_read_packet(apv->cbc, au, packet);
     if (err < 0) {
         av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n");
-        return err;
+        goto fail;
     }
 
     for (int i = 0; i < au->nb_units; i++) {
diff --git a/libavcodec/cbs_apv_syntax_template.c b/libavcodec/cbs_apv_syntax_template.c
index fefffe17b7..b6681681d4 100644
--- a/libavcodec/cbs_apv_syntax_template.c
+++ b/libavcodec/cbs_apv_syntax_template.c
@@ -113,12 +113,18 @@ static int FUNC(tile_info)(CodedBitstreamContext *ctx, RWContext *rw,
                            const APVRawFrameHeader *fh)
 {
     CodedBitstreamAPVContext *priv = ctx->priv_data;
+    int frame_width_in_mbs   = (fh->frame_info.frame_width  + 15) / 16;
+    int frame_height_in_mbs  = (fh->frame_info.frame_height + 15) / 16;
+    uint32_t min_tile_width  = FFMAX(APV_MIN_TILE_WIDTH_IN_MBS,
+                                     (frame_width_in_mbs + APV_MAX_TILE_COLS - 1) /
+                                     APV_MAX_TILE_COLS);
+    uint32_t min_tile_height = FFMAX(APV_MIN_TILE_HEIGHT_IN_MBS,
+                                     (frame_height_in_mbs + APV_MAX_TILE_ROWS - 1) /
+                                     APV_MAX_TILE_ROWS);
     int err;
 
-    u(20, tile_width_in_mbs,
-      APV_MIN_TILE_WIDTH_IN_MBS,  MAX_UINT_BITS(20));
-    u(20, tile_height_in_mbs,
-      APV_MIN_TILE_HEIGHT_IN_MBS, MAX_UINT_BITS(20));
+    u(20, tile_width_in_mbs,  min_tile_width,  MAX_UINT_BITS(20));
+    u(20, tile_height_in_mbs, min_tile_height, MAX_UINT_BITS(20));
 
     ub(1, tile_size_present_in_fh_flag);
 
-- 
2.47.2

_______________________________________________
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".

[FFmpeg-devel] [PATCH 5/6] apv_entropy: Improve robustness to bitstream errors

[Mark Thompson]

---
 libavcodec/apv_entropy.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/libavcodec/apv_entropy.c b/libavcodec/apv_entropy.c
index 00e0b4fbdf..a5648c09b4 100644
--- a/libavcodec/apv_entropy.c
+++ b/libavcodec/apv_entropy.c
@@ -84,6 +84,14 @@ static unsigned int apv_read_vlc(GetBitContext *gbc, int k_param,
         next_bits = show_bits(gbc, 16);
         leading_zeroes = 15 - av_log2(next_bits);
 
+        if (leading_zeroes == 0) {
+            // This can't happen mid-stream because the lookup would
+            // have resolved a leading one into a shorter code, but it
+            // can happen if we are hitting the end of the buffer.
+            // Return an invalid code to propagate as an error.
+            return APV_MAX_TRANS_COEFF + 1;
+        }
+
         skip_bits(gbc, leading_zeroes + 1);
 
         return (2 << k_param) +
@@ -182,6 +190,14 @@ int ff_apv_entropy_decode_block(int16_t *coeff,
                 else
                     level = abs_ac_coeff_minus1 + 1;
 
+                if (level < APV_MIN_TRANS_COEFF ||
+                    level > APV_MAX_TRANS_COEFF) {
+                    av_log(state->log_ctx, AV_LOG_ERROR,
+                           "Out-of-range AC coefficient value: %d "
+                           "(from prev_level %d abs_ac_coeff_minus1 %d sign_ac_coeff %d)\n",
+                           level, prev_level, abs_ac_coeff_minus1, sign_ac_coeff);
+                }
+
                 coeff[ff_zigzag_direct[scan_pos]] = level;
 
                 prev_level = abs_ac_coeff_minus1 + 1;
-- 
2.47.2

_______________________________________________
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".

[FFmpeg-devel] [PATCH 6/6] cbs_apv: Check tile component sizes

[Mark Thompson]

It was possible for the buffer pointers for the last tile to go over the
end of the unit buffer leading to a read overflow during decode of the
macroblock layer.  Check all tile component sizes to prevent this case
and also catch related tile size mismatch errors earlier.
---
 libavcodec/cbs_apv_syntax_template.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/libavcodec/cbs_apv_syntax_template.c b/libavcodec/cbs_apv_syntax_template.c
index b6681681d4..ca66349141 100644
--- a/libavcodec/cbs_apv_syntax_template.c
+++ b/libavcodec/cbs_apv_syntax_template.c
@@ -189,10 +189,12 @@ static int FUNC(frame_header)(CodedBitstreamContext *ctx, RWContext *rw,
 }
 
 static int FUNC(tile_header)(CodedBitstreamContext *ctx, RWContext *rw,
-                             APVRawTileHeader *current, int tile_idx)
+                             APVRawTileHeader *current,
+                             int tile_idx, uint32_t tile_size)
 {
     const CodedBitstreamAPVContext *priv = ctx->priv_data;
     uint16_t expected_tile_header_size;
+    uint32_t tile_size_remaining;
     uint8_t max_qp;
     int err;
 
@@ -203,8 +205,10 @@ static int FUNC(tile_header)(CodedBitstreamContext *ctx, RWContext *rw,
 
     u(16, tile_index, tile_idx, tile_idx);
 
+    tile_size_remaining = tile_size - current->tile_header_size;
     for (int c = 0; c < priv->num_comp; c++) {
-        us(32, tile_data_size[c], 1, MAX_UINT_BITS(32), 1, c);
+        us(32, tile_data_size[c], 1, tile_size_remaining, 1, c);
+        tile_size_remaining -= current->tile_data_size[c];
     }
 
     max_qp = 3 + priv->bit_depth * 6;
@@ -218,12 +222,14 @@ static int FUNC(tile_header)(CodedBitstreamContext *ctx, RWContext *rw,
 }
 
 static int FUNC(tile)(CodedBitstreamContext *ctx, RWContext *rw,
-                      APVRawTile *current, int tile_idx)
+                      APVRawTile *current,
+                      int tile_idx, uint32_t tile_size)
 {
     const CodedBitstreamAPVContext *priv = ctx->priv_data;
     int err;
 
-    CHECK(FUNC(tile_header)(ctx, rw, &current->tile_header, tile_idx));
+    CHECK(FUNC(tile_header)(ctx, rw, &current->tile_header,
+                            tile_idx, tile_size));
 
     for (int c = 0; c < priv->num_comp; c++) {
         uint32_t comp_size = current->tile_header.tile_data_size[c];
@@ -257,7 +263,8 @@ static int FUNC(frame)(CodedBitstreamContext *ctx, RWContext *rw,
     for (int t = 0; t < priv->tile_info.num_tiles; t++) {
         us(32, tile_size[t], 10, MAX_UINT_BITS(32), 1, t);
 
-        CHECK(FUNC(tile)(ctx, rw, &current->tile[t], t));
+        CHECK(FUNC(tile)(ctx, rw, &current->tile[t],
+                         t, current->tile_size[t]));
     }
 
     CHECK(FUNC(filler)(ctx, rw, &current->filler));
-- 
2.47.2

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/6] apv_decode: Fix memory leak on decode error

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 1117 bytes --]

On 5/3/2025 2:55 PM, Mark Thompson wrote:
> ---
>   libavcodec/apv_decode.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/apv_decode.c b/libavcodec/apv_decode.c
> index b1e1db7d64..2a59c9b25d 100644
> --- a/libavcodec/apv_decode.c
> +++ b/libavcodec/apv_decode.c
> @@ -377,7 +377,7 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>           case APV_PBU_PRIMARY_FRAME:
>               err = apv_decode(avctx, frame, pbu->content);
>               if (err < 0)
> -                return err;
> +                goto fail;
>               *got_frame = 1;
>               break;
>           case APV_PBU_METADATA:
> @@ -412,8 +412,11 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>       }
>   
>       ff_cbs_fragment_reset(au);
> -
>       return packet->size;
> +
> +fail:
> +    ff_cbs_fragment_reset(au);
> +    return err;
>   }
>   
>   const FFCodec ff_apv_decoder = {

nit: could instead make it

     err = packet->size;
fail:
     ff_cbs_fragment_reset(au);
     return err;


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/6] apv_decode: Improve reporting of decode errors

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 377 bytes --]

On 5/3/2025 2:55 PM, Mark Thompson wrote:
> +fail:
> +    av_log(avctx, AV_LOG_VERBOSE,
> +           "Decode error in tile %d component %d.\n",
> +           tile_index, comp_index);
> +    atomic_fetch_add(&apv->tile_errors, 1);

atomic_fetch_add_explicit(&apv->tile_errors, 1, memory_order_relaxed);

You don't care about order, just atomicity of the operation.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/6] apv_decode: Fix memory leak on decode error

[Mark Thompson]

On 03/05/2025 23:46, James Almer wrote:
> On 5/3/2025 2:55 PM, Mark Thompson wrote:
>> ---
>>   libavcodec/apv_decode.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/apv_decode.c b/libavcodec/apv_decode.c
>> index b1e1db7d64..2a59c9b25d 100644
>> --- a/libavcodec/apv_decode.c
>> +++ b/libavcodec/apv_decode.c
>> @@ -377,7 +377,7 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>>           case APV_PBU_PRIMARY_FRAME:
>>               err = apv_decode(avctx, frame, pbu->content);
>>               if (err < 0)
>> -                return err;
>> +                goto fail;
>>               *got_frame = 1;
>>               break;
>>           case APV_PBU_METADATA:
>> @@ -412,8 +412,11 @@ static int apv_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>>       }
>>         ff_cbs_fragment_reset(au);
>> -
>>       return packet->size;
>> +
>> +fail:
>> +    ff_cbs_fragment_reset(au);
>> +    return err;
>>   }
>>     const FFCodec ff_apv_decoder = {
> 
> nit: could instead make it
> 
>     err = packet->size;
> fail:
>     ff_cbs_fragment_reset(au);
>     return err;
> 
Set applied with this change and always using explicit memory ordering.

Thanks,

- Mark

_______________________________________________
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".