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.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> LGTM thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
