diff mbox series

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

Message ID 20220610231045.9760-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec/fmvc: Move frame allocation to a later stage | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer June 10, 2022, 11:10 p.m. UTC
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/fmvc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul B Mahol June 11, 2022, 8:47 a.m. UTC | #1
Have you actually tested this "change" ?
Michael Niedermayer June 11, 2022, 2:55 p.m. UTC | #2
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 ?

thx

[...]
Paul B Mahol June 13, 2022, 8:04 a.m. UTC | #3
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.


>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The bravest are surely those who have the clearest vision
> of what is before them, glory and danger alike, and yet
> notwithstanding go out to meet it. -- Thucydides
> _______________________________________________
> 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".
>
Anton Khirnov June 13, 2022, 9:10 a.m. UTC | #4
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.
Paul B Mahol June 13, 2022, 9:34 a.m. UTC | #5
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.


>
> --
> Anton Khirnov
> _______________________________________________
> 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".
>
Anton Khirnov June 13, 2022, 9:47 a.m. UTC | #6
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.
Paul B Mahol June 13, 2022, 10:10 a.m. UTC | #7
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.



> --
> Anton Khirnov
> _______________________________________________
> 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".
>
Michael Niedermayer June 13, 2022, 7:13 p.m. UTC | #8
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

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/fmvc.c b/libavcodec/fmvc.c
index 912ad8fc82..de2bf828f4 100644
--- a/libavcodec/fmvc.c
+++ b/libavcodec/fmvc.c
@@ -614,7 +614,7 @@  static av_cold int decode_init(AVCodecContext *avctx)
     }
 
     s->bpp          = avctx->bits_per_coded_sample >> 3;
-    s->buffer_size  = avctx->width * avctx->height * 4;
+    s->buffer_size  = s->stride * avctx->height * 4;
     s->pbuffer_size = avctx->width * avctx->height * 4;
     s->buffer       = av_mallocz(s->buffer_size);
     s->pbuffer      = av_mallocz(s->pbuffer_size);