Re: [FFmpeg-devel] [PATCH 2/3] avcodec/fmvc: buffer size is stride based not 4*width

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 5606 bytes --]

On Sun, Sep 04, 2022 at 11:42:49PM +0200, Michael Niedermayer wrote:
> On Fri, Sep 02, 2022 at 06:48:57PM +0200, Paul B Mahol wrote:
> > On Fri, Sep 2, 2022 at 6:32 PM Michael Niedermayer <michael@niedermayer.cc>
> > wrote:
> > 
> > > On Mon, Jun 13, 2022 at 09:13:19PM +0200, Michael Niedermayer wrote:
> > > > On Mon, Jun 13, 2022 at 12:10:44PM +0200, Paul B Mahol wrote:
> > > > > On Mon, Jun 13, 2022 at 11:48 AM Anton Khirnov <anton@khirnov.net>
> > > wrote:
> > > > >
> > > > > > Quoting Paul B Mahol (2022-06-13 11:34:44)
> > > > > > > On Mon, Jun 13, 2022 at 11:10 AM Anton Khirnov <anton@khirnov.net>
> > > > > > wrote:
> > > > > > >
> > > > > > > > Quoting Paul B Mahol (2022-06-13 10:04:04)
> > > > > > > > > On Sat, Jun 11, 2022 at 4:55 PM Michael Niedermayer <
> > > > > > > > michael@niedermayer.cc>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > On Sat, Jun 11, 2022 at 10:47:57AM +0200, Paul B Mahol wrote:
> > > > > > > > > > > Have you actually tested this "change" ?
> > > > > > > > > >
> > > > > > > > > > On every file i found
> > > > > > > > > > 6-methyl-5-hepten-2-one-CC-db_small.avi
> > > > > > > > > > fmvcVirtualDub_small.avi
> > > > > > > > > > skrzyzowanie4.avi
> > > > > > > > > > fmvc-poc.avi
> > > > > > > > > >
> > > > > > > > > > are there any other files i should test it on ?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes, the ones where stride != width.
> > > > > > > >
> > > > > > > > Give examples of such files then. And add more tests.
> > > > > > > >
> > > > > > > > You really should try to be more helpful if you care about this
> > > code
> > > > > > > > working.
> > > > > > >
> > > > > > >
> > > > > > > Code works perfectly from start. There are always attempts to
> > > break it.
> > > > > > > Your attempts to belittle my work are futile.
> > > > > >
> > > > > > Perfect code should live in an external repository that is locked
> > > > > > against modification.
> > > > > >
> > > > > > The ffmpeg repository is only for imperfect code that evolves with
> > > time,
> > > > > > and so requires changes.
> > > > > >
> > > > > >
> > > > > I dunno what Michael attempts to fix. Decoder works fine with valid
> > > files.
> > > > > I doubt that encoder would encode random bytes or padding into valid
> > > file
> > > > > bitstream.
> > > >
> > > > the stride*4 / width*4 change was because of 2 things.
> > > > first with AV_PIX_FMT_BGR24 the data stored is not width*4
> > > >
> > > > stride is in units of 4 bytes for some reason, so stride*4
> > > > fixes this
> > > > The 2nd issue is that the code addresses it by "s->stride * 4"
> > > > so the buffer allocation should be stride*4 if we belive the
> > > > other code is correct
> > > >
> > > >         src = s->buffer;
> > > > ...
> > > >         for (y = 0; y < avctx->height; y++) {
> > > > ...
> > > >             src += s->stride * 4;
> > > >
> > > > width*4 works because its bigger than stride*4 for BGR24 which is what
> > > all
> > > > samples i have use.
> > > >
> > > > also
> > > >         ssrc = s->buffer;
> > > >         ...
> > > >         for (y = 0; y < avctx->height; y++) {
> > > >             ...
> > > >             ssrc += s->stride * 4;
> > > > and
> > > >         dst = (uint32_t *)s->buffer;
> > > >
> > > >         for (block = 0, y = 0; y < s->yb; y++) {
> > > >             int block_h = s->blocks[block].h;
> > > >             uint32_t *rect = dst;
> > > >
> > > >             for (x = 0; x < s->xb; x++) {
> > > >                 int block_w = s->blocks[block].w;
> > > >                 uint32_t *row = dst;
> > > >
> > > >                 block_h = s->blocks[block].h;
> > > >                 if (s->blocks[block].xor) {
> > > >                     for (k = 0; k < block_h; k++) {
> > > >                         uint32_t *column = dst;
> > > >                         for (l = 0; l < block_w; l++)
> > > >                             *dst++ ^= *src++;
> > > >                         dst = &column[s->stride];
> > > >                     }
> > > >                 }
> > > >                 dst = &row[block_w];
> > > >                 ++block;
> > > >             }
> > > >             dst = &rect[block_h * s->stride];
> > > >         }
> > > >
> > > > Again, if you have fmvc files with more odd widths or other pixel formats
> > > > these would be very welcome. I can just say the code as is in git is
> > > wrong
> > > > and the buffer size as is in git is wrong. I noticed this when i added
> > > > a check to see if the buffer is only partly filled and realized its
> > > > always partly filled even when the whole image is actually touched
> > >
> > > If there are no objections aka noone sees a bug in this then id like
> > > to apply this
> > >
> > 
> > Since when are partially filled buffers are bad thing?
> 
> - waste of memory
> - breaks subsequent patch
> - width and stride relate this way: 
>   s->stride = (avctx->width * avctx->bits_per_coded_sample + 31) / 32;
>   is width always bigger or equal ?
>   If not we might be accessing outside the array because access uses
>   stride, allocation width
> 
> Or one line awnser
> Since when is it a good thing to mismatch allocation and access?

any objections to this ?
i think this should be applied

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".