diff mbox series

[FFmpeg-devel,v2] lavc/ffv1: Properly check that the 4th and 5th quant tables are zeroes

Message ID 20200105144525.6716-1-derek.buitenhuis@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] lavc/ffv1: Properly check that the 4th and 5th quant tables are zeroes
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Derek Buitenhuis Jan. 5, 2020, 2:45 p.m. UTC
Currently, the decoder checks the 128th value of the 4th quant table during
while deriving the context on each sample, in order to speed itself up. This
is due to relying on the behavior of FFmpeg's FFV1 encoder, in which if that
value is zero, the entire 4th and 5th quant tables are assumed to be entirely
zero.

This does not match the FFV1 spec, which has no such restriction, and after
some discussion, it was decided to fix FFmpeg to abide by the spec, rather
than change the spec.

We will now check whether the 4th and 5th quant tables are zero properly,
by checking the 128th valye of both tables (which means they are zero due
to the way they're coded in the bitstream).

For further context, the FFV1 issue in question is located at:

    https://github.com/FFmpeg/FFV1/issues/169

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
 libavcodec/ffv1_template.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Jan. 5, 2020, 11:44 p.m. UTC | #1
On Sun, Jan 05, 2020 at 02:45:25PM +0000, Derek Buitenhuis wrote:
> Currently, the decoder checks the 128th value of the 4th quant table during
> while deriving the context on each sample, in order to speed itself up. This
> is due to relying on the behavior of FFmpeg's FFV1 encoder, in which if that
> value is zero, the entire 4th and 5th quant tables are assumed to be entirely
> zero.
> 
> This does not match the FFV1 spec, which has no such restriction, and after
> some discussion, it was decided to fix FFmpeg to abide by the spec, rather
> than change the spec.
> 
> We will now check whether the 4th and 5th quant tables are zero properly,
> by checking the 128th valye of both tables (which means they are zero due
> to the way they're coded in the bitstream).
> 
> For further context, the FFV1 issue in question is located at:
> 
>     https://github.com/FFmpeg/FFV1/issues/169
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
>  libavcodec/ffv1_template.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

LGTM

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/ffv1_template.c b/libavcodec/ffv1_template.c
index f2ab93313a..c5f61b0182 100644
--- a/libavcodec/ffv1_template.c
+++ b/libavcodec/ffv1_template.c
@@ -37,7 +37,7 @@  static inline int RENAME(get_context)(PlaneContext *p, TYPE *src,
     const int RT = last[1];
     const int L  = src[-1];
 
-    if (p->quant_table[3][127]) {
+    if (p->quant_table[3][127] || p->quant_table[4][127]) {
         const int TT = last2[0];
         const int LL = src[-2];
         return p->quant_table[0][(L - LT) & 0xFF] +