[FFmpeg-devel] [PATCH] snow: remove strange av_assert2

[Michael Niedermayer]

On Tue, Jul 07, 2015 at 11:32:59PM +0200, Andreas Cadhalpun wrote:
> On 06.07.2015 02:40, Michael Niedermayer wrote:
> > On Sun, Jul 05, 2015 at 09:11:44PM +0200, Andreas Cadhalpun wrote:
> >> Can you explain how elements can be too larger to fit?
> > 
> > emulated_edge_mc() writes a block of width x height,
> 
> More precisely that's block_w x block_h.
> 

> > if stride < width this will not work as intended
> 
> Then an assert for stride > block_w might make sense in emulated_edge_mc,
> but not in add_yblock.

ok, added


> 
> > iam quite unsure if this is the intended thing for the assert to
> > check, i hthik there was more code that possibly was fixed
> 
> 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 ?


> After that I think 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.
> 

> So I think removing this assert is the correct way forward.

ok


lets hope i remember this  stuff correctly ....

> 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


>  * 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


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150708/1cf79075/attachment.sig>