On 08.07.2015 01:22, Michael Niedermayer wrote:
> On Tue, Jul 07, 2015 at 11:32:59PM +0200, Andreas Cadhalpun wrote:
>> I think the assert is a historic leftover:
>> Before commit cc884a35 src_stride > 7*MB_SIZE was necessary, because
>> the blocks were interleaved in the tmp buffer and the last block
>> was added with an offset of 6*MB_SIZE.
>> It was changed for src_stride <= 7*MB_SIZE to write the blocks
>> sequentially, hence the larger tmp_stride. (A comment about this
>> in the code would have been nice.)
>
> yes, should i add one or you want to ?
I added one to the patch. Updated version attached.
Does that sound good?
>> However, there are still some things in this code which are unclear for me:
>> * Where does the 5 in 'src_stride > 2*MB_SIZE + 5' come from?
>> Shouldn't that have been HTAPS_MAX-1, because that is added to block_h,
>> when calling emulated_edge_mc?
>> * Why the factor 2 in 'src_stride > 2*MB_SIZE + 5'?
>> Before commit cc884a35 the buffer size was 'src_stride*(b_h+5)' and
>> b_h is at maximum MB_SIZE, not 2*MB_SIZE.
>
> I dont remember trying to make the assert be exact just sufficient
> to detect problems, it was not intended to stay IIRC, so it would have
> been a waste of time to calculate exactly what the minimum size
> was
> also i think that we should only spend time on this investigation if
> we intend to work on the code. Its hardly worth for just removing
> the apparently unneeded assert.
> if you want to improve snow (the algorithm or implementation)
> then investigating every smal bit does make sense
OK.
>> * Why is tmp_step based on MB_SIZE and not (MB_SIZE + HTAPS_MAX-1)?
>> This 'HTAPS_MAX-1' is added to b_w/b_h when calling emulated_edge_mc.
>
> to keep things aligned in memory, it may help or be required for SIMD
> use
I suppose that's correct then.
Best regards,
Andreas
>From 7747ec5a7e319c05e28c6988caa84ad1f37fd301 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 9 Jul 2015 19:50:34 +0200
Subject: [PATCH] snow: remove an obsolete av_assert2
It asserts that the frame linesize is larger than 37, but it can be
smaller and decoding such frames works.
Before commit cc884a35 src_stride > 7*MB_SIZE was necessary, because the
blocks were interleaved in the tmp buffer and the last block was added
with an offset of 6*MB_SIZE.
It was changed for src_stride <= 7*MB_SIZE to write the blocks
sequentially, hence the larger tmp_step.
After that the assert was only necessary to make sure that the buffer
remained large enough.
Since commit bd2b6b33 s->scratchbuf is used as tmp buffer.
As part of commit 86e107a7 the minimal scratchbuf size was increased to
256*7*MB_SIZE, which is enough for any src_stride <= 7*MB_SIZE.
Also add a comment explaining the tmp_step calculation.
Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
libavcodec/snow.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/snow.h b/libavcodec/snow.h
index 95e07cd..447859f 100644
--- a/libavcodec/snow.h
+++ b/libavcodec/snow.h
@@ -304,6 +304,8 @@ static av_always_inline void add_yblock(SnowContext *s, int sliced, slice_buffer
BlockNode *lb= lt+b_stride;
BlockNode *rb= lb+1;
uint8_t *block[4];
+ // When src_stride is large enough, it is possible to interleave the blocks.
+ // Otherwise the blocks are written sequentially in the tmp buffer.
int tmp_step= src_stride >= 7*MB_SIZE ? MB_SIZE : MB_SIZE*src_stride;
uint8_t *tmp = s->scratchbuf;
uint8_t *ptmp;
@@ -347,8 +349,6 @@ static av_always_inline void add_yblock(SnowContext *s, int sliced, slice_buffer
if(b_w<=0 || b_h<=0) return;
- av_assert2(src_stride > 2*MB_SIZE + 5);
-
if(!sliced && offset_dst)
dst += src_x + src_y*dst_stride;
dst8+= src_x + src_y*src_stride;
--
2.1.4
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
