[FFmpeg-devel] [PATCH 1/3] avcodec/fmvc: Move frame allocation to a later stage

[FFmpeg-devel] [PATCH 1/3] avcodec/fmvc: Move frame allocation to a later stage

[Michael Niedermayer]

This way more things are checked before allocation

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/fmvc.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/libavcodec/fmvc.c b/libavcodec/fmvc.c
index 4abf6d7048..912ad8fc82 100644
--- a/libavcodec/fmvc.c
+++ b/libavcodec/fmvc.c
@@ -401,20 +401,17 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame,
     GetByteContext *gb = &s->gb;
     PutByteContext *pb = &s->pb;
     int ret, y, x;
+    int key_frame;
 
     if (avpkt->size < 8)
         return AVERROR_INVALIDDATA;
 
-    if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
-        return ret;
-
     bytestream2_init(gb, avpkt->data, avpkt->size);
     bytestream2_skip(gb, 2);
 
-    frame->key_frame = !!bytestream2_get_le16(gb);
-    frame->pict_type = frame->key_frame ? AV_PICTURE_TYPE_I : AV_PICTURE_TYPE_P;
+    key_frame = !!bytestream2_get_le16(gb);
 
-    if (frame->key_frame) {
+    if (key_frame) {
         const uint8_t *src;
         unsigned type, size;
         uint8_t *dst;
@@ -434,6 +431,12 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame,
             return AVERROR_PATCHWELCOME;
         }
 
+        if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
+            return ret;
+
+        frame->key_frame = 1;
+        frame->pict_type = AV_PICTURE_TYPE_I;
+
         src = s->buffer;
         dst = frame->data[0] + (avctx->height - 1) * frame->linesize[0];
         for (y = 0; y < avctx->height; y++) {
@@ -514,6 +517,12 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame,
             dst = &rect[block_h * s->stride];
         }
 
+        if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
+            return ret;
+
+        frame->key_frame = 0;
+        frame->pict_type = AV_PICTURE_TYPE_P;
+
         ssrc = s->buffer;
         ddst = frame->data[0] + (avctx->height - 1) * frame->linesize[0];
         for (y = 0; y < avctx->height; y++) {
-- 
2.17.1

_______________________________________________
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".

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

[Michael Niedermayer]

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

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);
-- 
2.17.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/3] avcodec/fmvc: Require key frames to fill a non trivial part of the buffer

[Michael Niedermayer]

Keyframes filling only part of the buffer should theoretically be invalid
as they are not really keyframes.

Fixes: Timeout
Fixes: 47879/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FMVC_fuzzer-6258764937822208

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/fmvc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libavcodec/fmvc.c b/libavcodec/fmvc.c
index de2bf828f4..36990956e0 100644
--- a/libavcodec/fmvc.c
+++ b/libavcodec/fmvc.c
@@ -284,7 +284,7 @@ static int decode_type2(GetByteContext *gb, PutByteContext *pb)
         }
     }
 
-    return 0;
+    return bytestream2_get_bytes_left_p(pb);
 }
 
 static int decode_type1(GetByteContext *gb, PutByteContext *pb)
@@ -391,7 +391,7 @@ static int decode_type1(GetByteContext *gb, PutByteContext *pb)
         } while (len && bytestream2_get_bytes_left(&gbc) > 0);
     }
 
-    return 0;
+    return bytestream2_get_bytes_left_p(pb);
 }
 
 static int decode_frame(AVCodecContext *avctx, AVFrame *frame,
@@ -414,6 +414,7 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame,
     if (key_frame) {
         const uint8_t *src;
         unsigned type, size;
+        int left;
         uint8_t *dst;
 
         type = bytestream2_get_le16(gb);
@@ -423,14 +424,17 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame,
 
         bytestream2_init_writer(pb, s->buffer, s->buffer_size);
         if (type == 1) {
-            decode_type1(gb, pb);
+            left = decode_type1(gb, pb);
         } else if (type == 2){
-            decode_type2(gb, pb);
+            left = decode_type2(gb, pb);
         } else {
             avpriv_report_missing_feature(avctx, "Compression type %d", type);
             return AVERROR_PATCHWELCOME;
         }
 
+        if (left > s->buffer_size*4/5)
+            return AVERROR_INVALIDDATA;
+
         if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
             return ret;
 
-- 
2.17.1

_______________________________________________
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".

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

[Paul B Mahol]

Have you actually tested this "change" ?
_______________________________________________
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".

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

[Michael Niedermayer]

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

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

[...]

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

[-- 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".

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

[Paul B Mahol]

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".
>
_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
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".

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

[Paul B Mahol]

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".
>
_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
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".

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

[Paul B Mahol]

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".
>
_______________________________________________
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".

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

[Michael Niedermayer]

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

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

[-- 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".

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

[Michael Niedermayer]

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

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange

[-- 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".

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

[Paul B Mahol]

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?

>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> What does censorship reveal? It reveals fear. -- Julian Assange
> _______________________________________________
> 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".
>
_______________________________________________
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".

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

[Michael Niedermayer]

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

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?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Nations do behave wisely once they have exhausted all other alternatives. 
-- Abba Eban

[-- 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".

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/fmvc: Move frame allocation to a later stage

[Michael Niedermayer]

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

On Sat, Jun 11, 2022 at 01:10:43AM +0200, Michael Niedermayer wrote:
> This way more things are checked before allocation
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/fmvc.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)

will apply

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.

[-- 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".

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

[Michael Niedermayer]

[-- 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".