[PATCH 1/3] avcodec/fmvc: Move frame allocation to a later stage
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
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
Have you actually tested this "change" ?
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
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".
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
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".
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
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".
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.
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
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".
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
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
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
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.
participants (3)
-
Anton Khirnov -
Michael Niedermayer -
Paul B Mahol