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

[Michael Niedermayer]

On Thu, Jul 09, 2015 at 08:16:29PM +0200, Andreas Cadhalpun wrote:
> 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

>  snow.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 62ba6bae883891f9953478d52d591e5f16aa759e  0001-snow-remove-an-obsolete-av_assert2.patch
> From 7747ec5a7e319c05e28c6988caa84ad1f37fd301 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at 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.Cadhalpun at googlemail.com>

LGTM

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20150710/d9ff6160/attachment.sig>