Re: [FFmpeg-devel] [PATCH 1/2] libavcodec/zmbv: change 24-bit decoder channel order, from RGB24 to BGR24
> On 2 Apr 2019, at 16:48, Tomas Härdin wrote: > > mån 2019-04-01 klockan 13:29 +0100 skrev Matthew Fearnley: >>>>> On 1 Apr 2019, at 10:28, Tomas Härdin wrote: >>> >>> fre 2019-03-29 klockan 22:23 + skrev Matthew Fearnley: >>>>>> On Wed, 27 Mar 2019 at 09:42, Tomas Härdin wrote: >>>>>> Additional minor fix: use PTRDIFF_SPECIFIER for `src - c->decomp_buf`. >>>>>> Other bit depths saw this change in ced0d6c14d, but this instance was >>>>>> missed, presumably because of the #ifdef block. >>>>> >>>>> I think it'd be best to split this off into its own patch, even if it's >>>>> trivial >>>>> >>>> >>>> Yeah, I think you're right. >>>> I'm attaching two patches here, if that works.. >>> >>> You got the commit messages mixed up :) Otherwise they look OK >> >> Doh! >> Do I need to correct that, or is it easy enough for you to fix up manually? > > Fixed and pushed. Thanks so much. By the way, is there a recommended way to enable #ifdef blocks when building ffmpeg? I’ve been adding #defines of the top of the source file, which does the job, but it feels like a bit of a kludge from a source-control perspective. > > /Tomas > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] libavcodec/zmbv: change 24-bit decoder channel order, from RGB24 to BGR24
> On 1 Apr 2019, at 10:28, Tomas Härdin wrote: > > fre 2019-03-29 klockan 22:23 +0000 skrev Matthew Fearnley: >>>> On Wed, 27 Mar 2019 at 09:42, Tomas Härdin wrote: >>>> Additional minor fix: use PTRDIFF_SPECIFIER for `src - c->decomp_buf`. >>>> Other bit depths saw this change in ced0d6c14d, but this instance was >>>> missed, presumably because of the #ifdef block. >>> >>> I think it'd be best to split this off into its own patch, even if it's >>> trivial >>> >> >> Yeah, I think you're right. >> I'm attaching two patches here, if that works.. > > You got the commit messages mixed up :) Otherwise they look OK Doh! Do I need to correct that, or is it easy enough for you to fix up manually? > > /Tomas > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] libavcodec/zmbv: change 24-bit decoder channel order, from RGB24 to BGR24
On Wed, 27 Mar 2019 at 09:42, Tomas Härdin wrote: > tis 2019-03-26 klockan 22:13 + skrev Matthew Fearnley: > > This brings the channel order in line with that used in 32-bit mode > (BGR0). > > > > 24-bit decoding is disabled by default (#ifdef ZMBV_ENABLE_24BPP), and no > > prior encoders or sample videos are known to exist for this bit depth, so > > I consider this change in implementation is unlikely to affect anyone. > > > > The decision has been made in agreement with the DOSBox Development Team > > > (dosbox.c...@gmail.com), specifically with harekiet, who wrote the > original > > codec. > > I can confirm this > > > Additional minor fix: use PTRDIFF_SPECIFIER for `src - c->decomp_buf`. > > Other bit depths saw this change in ced0d6c14d, but this instance was > > missed, presumably because of the #ifdef block. > > I think it'd be best to split this off into its own patch, even if it's > trivial > Yeah, I think you're right. I'm attaching two patches here, if that works.. > > /Tomas > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". From f78dc4276dd0517332bafdf99c7ce522aeb7cac2 Mon Sep 17 00:00:00 2001 From: Matthew Fearnley Date: Tue, 26 Mar 2019 19:44:23 + Subject: [PATCH 1/3] libavcodec/zmbv: change 24-bit decoder channel order, from RGB24 to BGR24 This brings the channel order in line with that used in 32-bit mode (BGR0). 24-bit decoding is disabled by default (#ifdef ZMBV_ENABLE_24BPP), and no prior encoders or sample videos are known to exist for this bit depth, so I consider this change in implementation is unlikely to affect anyone. The decision has been made in agreement with the DOSBox Development Team (dosbox.c...@gmail.com), specifically with harekiet, who wrote the original codec. --- libavcodec/zmbv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c index e07009d0fb..71ec2cd424 100644 --- a/libavcodec/zmbv.c +++ b/libavcodec/zmbv.c @@ -303,7 +303,7 @@ static int zmbv_decode_xor_24(ZmbvContext *c) prev += stride * c->bh; } if (src - c->decomp_buf != c->decomp_len) -av_log(c->avctx, AV_LOG_ERROR, "Used %i of %i bytes\n", +av_log(c->avctx, AV_LOG_ERROR, "Used %"PTRDIFF_SPECIFIER" of %i bytes\n", src-c->decomp_buf, c->decomp_len); return 0; } -- 2.17.1 From 905eb8b403281f180148bef50804af740d411b54 Mon Sep 17 00:00:00 2001 From: Matthew Fearnley Date: Fri, 29 Mar 2019 22:03:03 + Subject: [PATCH 2/3] libavcodec/zmbv: use PTRDIFF_SPECIFIER for `src - c->decomp_buf`. Other bit depths saw this change in ced0d6c14d, but this instance was presumably missed because of the #ifdef block. --- libavcodec/zmbv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c index 71ec2cd424..898b62d065 100644 --- a/libavcodec/zmbv.c +++ b/libavcodec/zmbv.c @@ -473,7 +473,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac c->bpp = 24; decode_intra = zmbv_decode_intra; c->decode_xor = zmbv_decode_xor_24; -avctx->pix_fmt = AV_PIX_FMT_RGB24; +avctx->pix_fmt = AV_PIX_FMT_BGR24; c->stride = c->width * 3; break; #endif //ZMBV_ENABLE_24BPP -- 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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/2] libavcodec/zmbvenc: add support for 24-bit encoding, using pix_fmt BGR24.
Support is #ifdef'd out at this stage, using ZMBV_ENABLE_24BPP (like in
the zmbv.c decoder)
---
libavcodec/zmbvenc.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c
index 98029de5f6..48871758e0 100644
--- a/libavcodec/zmbvenc.c
+++ b/libavcodec/zmbvenc.c
@@ -340,6 +340,12 @@ static av_cold int encode_init(AVCodecContext *avctx)
c->fmt = ZMBV_FMT_16BPP;
c->bypp = 2;
break;
+#ifdef ZMBV_ENABLE_24BPP
+case AV_PIX_FMT_BGR24:
+c->fmt = ZMBV_FMT_24BPP;
+c->bypp = 3;
+break;
+#endif //ZMBV_ENABLE_24BPP
case AV_PIX_FMT_BGR0:
c->fmt = ZMBV_FMT_32BPP;
c->bypp = 4;
@@ -434,6 +440,9 @@ AVCodec ff_zmbv_encoder = {
.pix_fmts = (const enum AVPixelFormat[]) { AV_PIX_FMT_PAL8,
AV_PIX_FMT_RGB555LE,
AV_PIX_FMT_RGB565LE,
+#ifdef ZMBV_ENABLE_24BPP
+ AV_PIX_FMT_BGR24,
+#endif //ZMBV_ENABLE_24BPP
AV_PIX_FMT_BGR0,
AV_PIX_FMT_NONE },
};
--
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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/2] libavcodec/zmbv: change 24-bit decoder channel order, from RGB24 to BGR24
This brings the channel order in line with that used in 32-bit mode (BGR0). 24-bit decoding is disabled by default (#ifdef ZMBV_ENABLE_24BPP), and no prior encoders or sample videos are known to exist for this bit depth, so I consider this change in implementation is unlikely to affect anyone. The decision has been made in agreement with the DOSBox Development Team (dosbox.c...@gmail.com), specifically with harekiet, who wrote the original codec. Additional minor fix: use PTRDIFF_SPECIFIER for `src - c->decomp_buf`. Other bit depths saw this change in ced0d6c14d, but this instance was missed, presumably because of the #ifdef block. --- libavcodec/zmbv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c index e07009d0fb..898b62d065 100644 --- a/libavcodec/zmbv.c +++ b/libavcodec/zmbv.c @@ -303,7 +303,7 @@ static int zmbv_decode_xor_24(ZmbvContext *c) prev += stride * c->bh; } if (src - c->decomp_buf != c->decomp_len) -av_log(c->avctx, AV_LOG_ERROR, "Used %i of %i bytes\n", +av_log(c->avctx, AV_LOG_ERROR, "Used %"PTRDIFF_SPECIFIER" of %i bytes\n", src-c->decomp_buf, c->decomp_len); return 0; } @@ -473,7 +473,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac c->bpp = 24; decode_intra = zmbv_decode_intra; c->decode_xor = zmbv_decode_xor_24; -avctx->pix_fmt = AV_PIX_FMT_RGB24; +avctx->pix_fmt = AV_PIX_FMT_BGR24; c->stride = c->width * 3; break; #endif //ZMBV_ENABLE_24BPP -- 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/zmbvenc: Add support for RGB formats
> On 12 Mar 2019, at 11:46, Tomas Härdin wrote: > > tis 2019-03-12 klockan 10:27 +0000 skrev Matthew Fearnley: >>> On 11 Mar 2019, at 10:37, Tomas Härdin wrote: >>> >>> >>> There's some justification for adding sub-8bpp, like BMP. bmp.c >>> converts all of them except GRAY8 to PAL8. Bitdepths besides 1, 4 >>> and 8 >>> don't work at all. >>> >>> One way to at least allow both the bmp and zmbv encoders to do sub- >>> 8bpp >>> from PAL8 would be to keep track of the maximum number of colors in >>> some appropriate struct. >>> >>> Adding proper sub-8bpp support would involve a lot of libsws >>> headache I >>> suspect. >> >> It occurs to me that adding sub-8bpp has some implications: >> >> My current understanding (I could be wrong) is that FFmpeg tends to >> detect the pix_fmt based on the first frame. If FFmpeg detects the >> first frame as e.g. PAL4, and chooses that as its output, that means >> the rest of the video will have to be encodable as PAL4, otherwise it >> (obviously) won’t be encoded properly. >> >> So adding a PAL4 format puts a new constraint on encoders (inside and >> outside FFmpeg) to not encode frames in a way that looks like PAL4, >> unless the whole video will be encodable that way. > > Yes, FFmpeg will probe the initial format of the video and audio. > Nothing says these are constant. There are FATE samples specifically > for files that change resolution. Since ZMBV is a DOS capture codec, > and DOS programs frequently change resolution and colordepth, this is > indeed something we have to think about. Example: DOS boots into mode > 3h, 80x25 16-color text. A recording may start in this mode, then > switch to mode 13h (320x200 256 colors graphics), if I understand the > format correctly. DOSBox actually avoids this issue by outputting to a new file whenever it detects a change in colour depth, dimensions or FPS. So in practice, these remain constant over a single video. I’ve tried stitching two AVI files together (palette + RGB, same dimensions), and found that if a palette-capable format like ‘png’ is chosen, all of the resulting AVI file is encoded with a palette, and RGB sections are dithered. That possibly suggests FFmpeg doesn’t like changing format halfway through videos, for AVI at least? > >> If FFmpeg supports PAL8 only, then it can be tempting to optimise >> videos to encode as sub-8bpp whenever possible, knowing they will >> always (in FFmpeg at least) decode to PAL8. But this could break >> format detection for tools outside FFmpeg, if they choose to add sub- >> 8bpp support. >> >> The safest thing FFmpeg can do is to always decode sub-8bpp to PAL8, >> and to emit PAL8 frames as exactly 8bpp (where applicable). It could >> still offer encoding formats for PAL1/2/4, but these formats could >> only be detected by scanning the whole video. >> >> The suggestion of bits_per_raw_sample sounds interesting. What would >> that look like in practice? > > The decoder would set bits_per_raw_sample on every keyframe. It and > resolution might change during the course of a ZMBV file, as explained > above. > > I think frivolously changing the bitdepth is a bad idea, at least > inside the encoder. If the encoder is fed with changing bitdepths then > it's a different story. > >>> Just a small thing to be clear: ZMBV_ENABLE_24BPP is not defined >>> anywhere, so we're free to do however we want with it. It's not >>> going >>> to break anyone's workflow unless they were foolish enough to >>> encode >>> 24-bit ZMBVs outside of the non-existing spec. >> >> True. But they might be understandably puzzled if they encode as 24- >> bit, and then find the channels swapped when they decode. > > Yeah, that's why we'd need to get this nailed down > >> Thanks for writing the email to the DOSBox crew. >> If they choose a channel order, then we have good grounds for fixing >> the encoder (if need be), and implementing the decoder in the same >> way. >> It occurs to me that they might (in theory) also want to specify 2/4 >> byte alignment on RGB, like with the MVs. My gut says there’d be >> very little benefit though, and it would only be seen with strange >> video / block widths. >> >> It also occurs to me this will may warrant a version bump in the >> format, to give an easy error case for decoders that don’t expect it. >> Particularly if our decoder has to redefine its channel order. > > Are there any decoders besides ours and dosbox's? I don’t know of a
Re: [FFmpeg-devel] [PATCH] libavcodec/zmbvenc: Add support for RGB formats
> On 11 Mar 2019, at 10:37, Tomas Härdin wrote: > > fre 2019-03-08 klockan 21:39 +0000 skrev Matthew Fearnley: >>>> On Fri, 8 Mar 2019 at 18:07, Carl Eugen Hoyos wrote: >>>> 2019-03-08 15:04 GMT+01:00, Tomas Härdin : >>>> >>>> Maybe we could coordinate 1/2/4/24-bit support with the >>> >>> I believe FFmpeg cannot support 1/2/4 bit for encoding. >>> >> >> As far as I can see, FFmpeg has very limited support for bit depths less >> than 8. I think there are basically two formats (plus variants), with >> fixed "palettes": >> 1bpp: MONO_BLACK / MONO_WHITE (black/white only) >> 4bpp: RGB4[_BYTE], BGR4[_BYTE] (RGB 1:2:1, 16 colours - I presume red/blue >> would be 0,255; green would be 0,85,170,255) >> >> If the ZMBV formats were defined, these might be worth encoder adding >> support for. >> (Practically speaking though, it would be a slight pain, because the >> encoder would do the work in 8bpp and pack/unpack as needed.) >> But with PAL8 being the only format allowing a free palette, all sub-8bpp >> formats would have to decode to that, so they wouldn't round-trip. > > There's some justification for adding sub-8bpp, like BMP. bmp.c > converts all of them except GRAY8 to PAL8. Bitdepths besides 1, 4 and 8 > don't work at all. > > One way to at least allow both the bmp and zmbv encoders to do sub-8bpp > from PAL8 would be to keep track of the maximum number of colors in > some appropriate struct. > > Adding proper sub-8bpp support would involve a lot of libsws headache I > suspect. It occurs to me that adding sub-8bpp has some implications: My current understanding (I could be wrong) is that FFmpeg tends to detect the pix_fmt based on the first frame. If FFmpeg detects the first frame as e.g. PAL4, and chooses that as its output, that means the rest of the video will have to be encodable as PAL4, otherwise it (obviously) won’t be encoded properly. So adding a PAL4 format puts a new constraint on encoders (inside and outside FFmpeg) to not encode frames in a way that looks like PAL4, unless the whole video will be encodable that way. If FFmpeg supports PAL8 only, then it can be tempting to optimise videos to encode as sub-8bpp whenever possible, knowing they will always (in FFmpeg at least) decode to PAL8. But this could break format detection for tools outside FFmpeg, if they choose to add sub-8bpp support. The safest thing FFmpeg can do is to always decode sub-8bpp to PAL8, and to emit PAL8 frames as exactly 8bpp (where applicable). It could still offer encoding formats for PAL1/2/4, but these formats could only be detected by scanning the whole video. The suggestion of bits_per_raw_sample sounds interesting. What would that look like in practice? >> (It should be possible to implement decoding to pal8 if >>> that doesn't work yet and if samples exist.) >>> >> >> No samples or specifications exist that I know of, so I don't plan to >> submit any patches to the decoder unless/until there is something to work >> with there. >> >>> dosbox devs? And maybe we should do something about >>>> the RGB24 thing in the decoder.. >> >> Yeah, I think talking with the DOSBox devs sounds like a potentially good >> idea. >> >>> >>> Do I understand correctly that no existing implementation >>> supports 24bit rgb? If that is correct, I believe FFmpeg >>> shouldn't add it (but this may only be me). >>> >> >> I agree that FFmpeg shouldn't add support for any formats that haven't been >> defined. >> As far as I know, the specifics of the 24-bit RGB format havn't been >> discussed anywhere, and there are no samples I know of. >> >> A likely specification of 24-bit is trivial enough to add support for, that >> I was originally planning to add it with an #ifdef (like in the decoder). >> But it wouldn't do to have contradictory channel orders proposed in the >> decoder and encoder, so I will leave that for now unless DOSBox will commit >> to one. >> >> I presume that FFmpeg generally doesn't like to set standards in media >> formats, only to implement existing ones. >> My personal feelings in this case would be to provide support that's >> disabled at compile-time if an official specification can be agreed, and to >> have support included by default if an independent implementation - or at >> least independent samples - are available that agree with the specification. > > Just a small thing to be clear: ZMBV_ENABLE_24BPP is not defined > anywhere, so we're free to do however we want with it. It's not going > to break anyone
Re: [FFmpeg-devel] [PATCH] libavcodec/zmbvenc: Add support for RGB formats
On Fri, 8 Mar 2019 at 18:07, Carl Eugen Hoyos wrote: > 2019-03-08 15:04 GMT+01:00, Tomas Härdin : > > tor 2019-03-07 klockan 14:42 +0000 skrev Matthew Fearnley: > >> This consists mostly of the following changes: > >> - add newly supported pixel formats (RGB555LE, RGB565LE, BGR0) > >> - select the ZMBV format (c->fmt) and bytes per pixel (c->bypp) based on > >> avctx->pix_fmt > >> - multiply widths/x-values by c->bypp, in places where bytes, not > pixels, > >> are > >> expected > >> - disable palette-writing code for non-palette pix_fmts > >> - make a note about histogram[]'s datatype (it could need increasing if > >> ZMBV_BLOCK is increased) > >> - adjust the c->score_tab length to take up to (and including) 4 times > the > >> number of pixels in a block > >> - initialise c->score_tab up to c->bypp * the number of pixels > >> > >> Note: the ZmbvFormat enum allows for additional bit depths: > >> - 1,2,4-bit (palette) > >> - 24-bit (RGB) > >> > >> At time of writing the specifics of these (e.g. channel order, bit > >> alignment) > >> are not currently defined, and DOSBox only implements support for > >> 8/15/16/32 > >> bpp. > >> One might expect the 24-bit format - if implemented - to be BGR24, to > have > >> the > >> same channel order as BGR0. > >> However, the decoder in zmbv.c has been guessed to use RGB24, so I have > >> chosen > >> to not contradict this, and omitted specific support for this format. > > > > Sounds good. > > Yes. > > > Maybe we could coordinate 1/2/4/24-bit support with the > > I believe FFmpeg cannot support 1/2/4 bit for encoding. > As far as I can see, FFmpeg has very limited support for bit depths less than 8. I think there are basically two formats (plus variants), with fixed "palettes": 1bpp: MONO_BLACK / MONO_WHITE (black/white only) 4bpp: RGB4[_BYTE], BGR4[_BYTE] (RGB 1:2:1, 16 colours - I presume red/blue would be 0,255; green would be 0,85,170,255) If the ZMBV formats were defined, these might be worth encoder adding support for. (Practically speaking though, it would be a slight pain, because the encoder would do the work in 8bpp and pack/unpack as needed.) But with PAL8 being the only format allowing a free palette, all sub-8bpp formats would have to decode to that, so they wouldn't round-trip. (It should be possible to implement decoding to pal8 if > that doesn't work yet and if samples exist.) > No samples or specifications exist that I know of, so I don't plan to submit any patches to the decoder unless/until there is something to work with there. > dosbox devs? And maybe we should do something about > > the RGB24 thing in the decoder.. > Yeah, I think talking with the DOSBox devs sounds like a potentially good idea. > > Do I understand correctly that no existing implementation > supports 24bit rgb? If that is correct, I believe FFmpeg > shouldn't add it (but this may only be me). > I agree that FFmpeg shouldn't add support for any formats that haven't been defined. As far as I know, the specifics of the 24-bit RGB format havn't been discussed anywhere, and there are no samples I know of. A likely specification of 24-bit is trivial enough to add support for, that I was originally planning to add it with an #ifdef (like in the decoder). But it wouldn't do to have contradictory channel orders proposed in the decoder and encoder, so I will leave that for now unless DOSBox will commit to one. I presume that FFmpeg generally doesn't like to set standards in media formats, only to implement existing ones. My personal feelings in this case would be to provide support that's disabled at compile-time if an official specification can be agreed, and to have support included by default if an independent implementation - or at least independent samples - are available that agree with the specification. , Matthew ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavcodec/zmbvenc: Add support for RGB formats
This consists mostly of the following changes:
- add newly supported pixel formats (RGB555LE, RGB565LE, BGR0)
- select the ZMBV format (c->fmt) and bytes per pixel (c->bypp) based on
avctx->pix_fmt
- multiply widths/x-values by c->bypp, in places where bytes, not pixels, are
expected
- disable palette-writing code for non-palette pix_fmts
- make a note about histogram[]'s datatype (it could need increasing if
ZMBV_BLOCK is increased)
- adjust the c->score_tab length to take up to (and including) 4 times the
number of pixels in a block
- initialise c->score_tab up to c->bypp * the number of pixels
Note: the ZmbvFormat enum allows for additional bit depths:
- 1,2,4-bit (palette)
- 24-bit (RGB)
At time of writing the specifics of these (e.g. channel order, bit alignment)
are not currently defined, and DOSBox only implements support for 8/15/16/32
bpp.
One might expect the 24-bit format - if implemented - to be BGR24, to have the
same channel order as BGR0.
However, the decoder in zmbv.c has been guessed to use RGB24, so I have chosen
to not contradict this, and omitted specific support for this format.
---
libavcodec/zmbvenc.c | 104 ---
1 file changed, 77 insertions(+), 27 deletions(-)
diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c
index c9d50b6adf..98029de5f6 100644
--- a/libavcodec/zmbvenc.c
+++ b/libavcodec/zmbvenc.c
@@ -34,11 +34,29 @@
#include
+/* Frame header flags */
#define ZMBV_KEYFRAME 1
#define ZMBV_DELTAPAL 2
+/* Motion block width/height (maximum allowed value is 255)
+ * Note: histogram datatype in block_cmp() must be big enough to hold values
+ * up to (4 * ZMBV_BLOCK * ZMBV_BLOCK)
+ */
#define ZMBV_BLOCK 16
+/* Keyframe header format values */
+enum ZmbvFormat {
+ZMBV_FMT_NONE = 0,
+ZMBV_FMT_1BPP = 1,
+ZMBV_FMT_2BPP = 2,
+ZMBV_FMT_4BPP = 3,
+ZMBV_FMT_8BPP = 4,
+ZMBV_FMT_15BPP = 5,
+ZMBV_FMT_16BPP = 6,
+ZMBV_FMT_24BPP = 7,
+ZMBV_FMT_32BPP = 8
+};
+
/**
* Encoder context
*/
@@ -53,9 +71,11 @@ typedef struct ZmbvEncContext {
int pstride;
int comp_size;
int keyint, curfrm;
+int bypp;
+enum ZmbvFormat fmt;
z_stream zstream;
-int score_tab[ZMBV_BLOCK * ZMBV_BLOCK + 1];
+int score_tab[ZMBV_BLOCK * ZMBV_BLOCK * 4 + 1];
} ZmbvEncContext;
@@ -69,10 +89,11 @@ static inline int block_cmp(ZmbvEncContext *c, uint8_t
*src, int stride,
int sum = 0;
int i, j;
uint16_t histogram[256] = {0};
+int bw_bytes = bw * c->bypp;
/* Build frequency histogram of byte values for src[] ^ src2[] */
for(j = 0; j < bh; j++){
-for(i = 0; i < bw; i++){
+for(i = 0; i < bw_bytes; i++){
int t = src[i] ^ src2[i];
histogram[t]++;
}
@@ -81,7 +102,7 @@ static inline int block_cmp(ZmbvEncContext *c, uint8_t *src,
int stride,
}
/* If not all the xored values were 0, then the blocks are different */
-*xored = (histogram[0] < bw * bh);
+*xored = (histogram[0] < bw_bytes * bh);
/* Exit early if blocks are equal */
if (!*xored) return 0;
@@ -114,7 +135,7 @@ static int zmbv_me(ZmbvEncContext *c, uint8_t *src, int
sstride, uint8_t *prev,
/* Try previous block's MV (if not 0,0) */
if (mx0 || my0){
-tv = block_cmp(c, src, sstride, prev + mx0 + my0 * pstride, pstride,
bw, bh, );
+tv = block_cmp(c, src, sstride, prev + mx0 * c->bypp + my0 * pstride,
pstride, bw, bh, );
if(tv < bv){
bv = tv;
*mx = mx0;
@@ -129,7 +150,7 @@ static int zmbv_me(ZmbvEncContext *c, uint8_t *src, int
sstride, uint8_t *prev,
for(dx = -c->lrange; dx <= c->urange; dx++){
if(!dx && !dy) continue; // we already tested this block
if(dx == mx0 && dy == my0) continue; // this one too
-tv = block_cmp(c, src, sstride, prev + dx + dy * pstride, pstride,
bw, bh, );
+tv = block_cmp(c, src, sstride, prev + dx * c->bypp + dy *
pstride, pstride, bw, bh, );
if(tv < bv){
bv = tv;
*mx = dx;
@@ -165,9 +186,10 @@ FF_DISABLE_DEPRECATION_WARNINGS
avctx->coded_frame->key_frame = keyframe;
FF_ENABLE_DEPRECATION_WARNINGS
#endif
-chpal = !keyframe && memcmp(p->data[1], c->pal2, 1024);
-palptr = (uint32_t*)p->data[1];
+palptr = (avctx->pix_fmt == AV_PIX_FMT_PAL8) ? (uint32_t *)p->data[1] :
NULL;
+chpal = !keyframe && palptr && memcmp(palptr, c->pal2, 1024);
+
src = p->data[0];
prev = c->prev;
if(chpal){
@@ -181,19 +203,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
c->pal[i * 3 + 1] = tpal[1];
c->pal[i * 3 + 2] = tpal[2];
}
-memcpy(c->pal2, p->data[1], 1024);
+memcpy(c->pal2, palptr, 1024);
}
if(keyframe){
-for(i = 0; i < 256; i++){
-AV_WB24(c->pal+(i*3), palptr[i]);
+if (palptr){
+for(i = 0; i < 256; i++){
+
Re: [FFmpeg-devel] [PATCH 1/2] libavcodec/zmbvenc: block scoring improvements/bug fixes
On Thu, 21 Feb 2019 at 19:28, Michael Niedermayer wrote: > On Thu, Feb 21, 2019 at 11:19:08AM +0100, Tomas Härdin wrote: > > ons 2019-02-20 klockan 22:12 +0100 skrev Michael Niedermayer: > > > On Sat, Feb 09, 2019 at 01:10:20PM +, Matthew Fearnley wrote: > > > > - Improve block choices by counting 0-bytes in the entropy score > > > > - Make histogram use uint16_t type, to allow byte counts from 16*16 > > > > (current block size) up to 255*255 (maximum allowed 8bpp block size) > > > > - Make sure score table is big enough for a full block's worth of > bytes > > > > - Calculate *xored without using code in inner loop > > > > > > This should have been split into multiple changes > > > > Alas > > > > > > compression seems to become slightly worse from this change > > > > > > ./ffmpeg -i matrixbench_mpeg2.mpg -vframes 30 -vcodec zmbv -an -y > test-old.avi > > > ./ffmpeg -i matrixbench_mpeg2.mpg -vframes 30 -vcodec zmbv -an -y > test-new.avi > > > > > > -rw-r- 1 michael michael 1175466 Feb 20 22:06 test-new.avi > > > -rw-r- 1 michael michael 1174832 Feb 20 22:07 test-old.avi > > > > A whopping 0.05% change, > > no its just a tiny difference but i think it still would make sense to > understand it. > Generally a patch doing 4 improvments would be expected to improve things. > This was not a hand picked case. I mean its not as if I saw 10 cases > improving things and i picked the 11th. i looked at one case and that was > worse > > Also either way the patch should have been split, dont you agree ? > it also would make it much easier to within seconds identify which > change actually caused the 0.05% difference. (maybe avoiding this > discussion as it maybe would be obvious from that if this is a > ommision in the commit message a bug a rare outlier ...) > > Hi Michael. First, let me say thanks for your feedback on this patch. Let me address some of this.. - Yes: in retrospect, I think I should have split the patch. I struggled here to work out how to split up my changes, and spent too long staring too closely at the code to see this. I still perhaps have some way to go before I would be able to get a proper feel for the general FFmpeg development ecosystem. - To clear up the effects: the change from 'i=1' to 'i=0' was the only change that should make any difference in the output. The rest of the changes were to improve the speed of an inner loop, and to fix two bugs that happily cancelled each other out, but would have broken if the code were changed to emit larger blocks. These should not lead to changes in the blocks chosen or the final compression size. - Regarding the worse compression: let me start by stating that scoring on byte frequency alone will not always predict well how Deflate will work, since Deflate also uses pattern matching as well. Frequency-based block scoring will only ever be a heuristic. There may be scope for improving the scoring, but it would add a lot of complexity, and I think it would be easy to make things worse rather than better. (I found this in my brief experiments with including the frequencies from the previous block.) I think the main thing that persuaded me to make this particular change from 1 to 0 was that it gave a pure entropy-based score, and allowed me, mathematically, to use the same scoring table on both full and partial blocks, without having to worry about the effects of omitting the 0-bytes in the score in each case. I focused my testing on the available ZMBV samples, which were likely all generated by DOSBox, and my findings were that while not all movies were smaller, overall the gains outweighed the losses. I'm slightly sad to hear it didn't improve this real-world movie sample. But I think this level in this case is acceptable, and not outside the expected range of size difference that I've seen. And the improvements to the motion estimation range (from the other patch) should tend to lead to a compression improvement overall. Maybe I should have been clearer though, that the scoring changes won't bring universal improvements. And if someone looks into this and finds on average results are worsened by the change, I won't complain if it gets reverted. Thanks for your feedback, I appreciate it. Matthew > > with an MPEG source rather than the intended > > PAL8.. > > The input to the encoder is always PAL8. so i assume you mean > "old computer game material" vs. "realistic video". > > > > > > The flip side here is that we can now use larger block sizes and higher > > me_range without crashing the encoder. We can add a slower mode that > > tests a few different block sizes to see which one results in the > > smallest o
Re: [FFmpeg-devel] [PATCH 1/2] libavcodec/zmbvenc: block scoring improvements/bug fixes
On Thu, 21 Feb 2019 at 10:26, Tomas Härdin wrote: > ons 2019-02-20 klockan 23:33 +0100 skrev Carl Eugen Hoyos: > > > 2019-02-10 16:42 GMT+01:00, Tomas Härdin : > > > lör 2019-02-09 klockan 13:10 + skrev Matthew Fearnley: > > > > - Improve block choices by counting 0-bytes in the entropy score > > > > - Make histogram use uint16_t type, to allow byte counts from 16*16 > > > > (current block size) up to 255*255 (maximum allowed 8bpp block size) > > > > - Make sure score table is big enough for a full block's worth of > bytes > > > > - Calculate *xored without using code in inner loop > > > > --- > > > > libavcodec/zmbvenc.c | 22 -- > > > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > > > Passes FATE, looks good to me > > > > I believe you asked on irc about fate tests: > > Since such a test would depend on the zlib version, you cannot test > > exact output, you could only test a round-trip (assuming the codec > > really is lossless). > > Right, we'd need to embed a specific version of zlib. But we probably > don't want to do that, for many reasons. > > A dummy DEFLATE implementation that just passes the bytes along could > be useful. I know there's a mode in the DEFLATE format for blocks that > fail to compress. That way we could test many encoders that depend on > zlib, and their output should decode just fine. > I know the DEFLATE format fairly well, and a dummy DEFLATE implementation would certainly be possible. An uncompressed block can be up to 65535 bytes, and has a simple five-byte header (subject to some bit alignment issues, but they only come into play when other block types are also used). Here's a simple code example, the guts of which fit into <70 lines: https://gist.github.com/countingpine/602453d330ea055a4bcab90ca2a7ed02 It allows data to be compressed in chunks, allowing a stream to be split across frames, like in ZMBV. The first chunk needs the START flag, and the last chunk (if any) needs the FINISH flag. It also allows adding an Adler32 checksum - although ZMBV itself doesn't need this, because it never "finishes" the stream. The question would still remain though, of the best way to incorporate it into FFMEG. That's probably beyond my current knowledge.. Matthew /Tomas > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] zmbvenc: don't sum the entropy when blocks are equal
On Thu, 31 Jan 2019 at 15:00, Tomas Härdin wrote: > > > 1. The entropy calculation in block_cmp() omits the score of histogram[0] > > from the final sum. > > It's tempting to do this to bias the scores in favour of 0-bytes, but in > > reality, blocks with a majority of 0 (or any other byte) will already be > > naturally favoured by the entropy score anyway, and this bias will fail > to > > penalise blocks with an "average" (i.e. high entropy) number of 0's in > them. > > The exact implications are difficult to ponder, but in practical terms > this > > error does tend to produce worse results in the video compression. Not > > massively so, but it's still noticeable. > > Did you try combining the statistics of the current MV with the > statistics of the previous block, to bias the choice in favor of > similar bytes? Could work like a cheap order-1 entropy model. > I've had a go at this, but sadly, it seemed to adversely affect compression, producing larger files. Maybe it can be improved with some tweaking, or maybe there's a bug somewhere. But it feels to me like this approach on its own isn't enough to improve scoring. Maybe as part of something larger though.. Anyway, you can see the results of my efforts here: https://github.com/countingpine/FFmpeg/commit/prev_histogram. - Matthew ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] libavcodec/zmbvenc: block scoring improvements/bug fixes
- Improve block choices by counting 0-bytes in the entropy score
- Make histogram use uint16_t type, to allow byte counts from 16*16
(current block size) up to 255*255 (maximum allowed 8bpp block size)
- Make sure score table is big enough for a full block's worth of bytes
- Calculate *xored without using code in inner loop
---
libavcodec/zmbvenc.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c
index 4d9147657d..3df6e724c8 100644
--- a/libavcodec/zmbvenc.c
+++ b/libavcodec/zmbvenc.c
@@ -55,7 +55,7 @@ typedef struct ZmbvEncContext {
int keyint, curfrm;
z_stream zstream;
-int score_tab[256];
+int score_tab[ZMBV_BLOCK * ZMBV_BLOCK + 1];
} ZmbvEncContext;
@@ -69,20 +69,26 @@ static inline int block_cmp(ZmbvEncContext *c, uint8_t
*src, int stride,
{
int sum = 0;
int i, j;
-uint8_t histogram[256] = {0};
+uint16_t histogram[256] = {0};
-*xored = 0;
+/* Build frequency histogram of byte values for src[] ^ src2[] */
for(j = 0; j < bh; j++){
for(i = 0; i < bw; i++){
int t = src[i] ^ src2[i];
histogram[t]++;
-*xored |= t;
}
src += stride;
src2 += stride2;
}
-for(i = 1; i < 256; i++)
+/* If not all the xored values were 0, then the blocks are different */
+*xored = (histogram[0] < bw * bh);
+
+/* Exit early if blocks are equal */
+if (!*xored) return 0;
+
+/* Sum the entropy of all values */
+for(i = 0; i < 256; i++)
sum += c->score_tab[histogram[i]];
return sum;
@@ -278,7 +284,11 @@ static av_cold int encode_init(AVCodecContext *avctx)
int i;
int lvl = 9;
-for(i=1; i<256; i++)
+/* Entropy-based score tables for comparing blocks.
+ * Suitable for blocks up to (ZMBV_BLOCK * ZMBV_BLOCK) bytes.
+ * Scores are nonnegative, lower is better.
+ */
+for(i = 1; i <= ZMBV_BLOCK * ZMBV_BLOCK; i++)
c->score_tab[i] = -i * log2(i / (double)(ZMBV_BLOCK * ZMBV_BLOCK)) *
256;
c->avctx = avctx;
--
2.17.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] libavcodec/zmbvenc: motion estimation improvements/bug fixes:
- Clamp ME range to -64..63 (prevents corruption when me_range is too high)
- Allow MV's up to *and including* the positive range limit
- Allow out-of-edge ME by padding the prev buffer with a border of 0's
- Try previous MV before checking the rest (improves speed in some cases)
- More robust logic in code - ensure *mx,*my,*xored are updated together
---
libavcodec/zmbvenc.c | 64 +++-
1 file changed, 46 insertions(+), 18 deletions(-)
diff --git a/libavcodec/zmbvenc.c b/libavcodec/zmbvenc.c
index 3df6e724c8..e92193478b 100644
--- a/libavcodec/zmbvenc.c
+++ b/libavcodec/zmbvenc.c
@@ -45,11 +45,11 @@
typedef struct ZmbvEncContext {
AVCodecContext *avctx;
-int range;
+int lrange, urange;
uint8_t *comp_buf, *work_buf;
uint8_t pal[768];
uint32_t pal2[256]; //for quick comparisons
-uint8_t *prev;
+uint8_t *prev, *prev_buf;
int pstride;
int comp_size;
int keyint, curfrm;
@@ -61,7 +61,6 @@ typedef struct ZmbvEncContext {
/** Block comparing function
* XXX should be optimized and moved to DSPContext
- * TODO handle out of edge ME
*/
static inline int block_cmp(ZmbvEncContext *c, uint8_t *src, int stride,
uint8_t *src2, int stride2, int bw, int bh,
@@ -100,23 +99,42 @@ static inline int block_cmp(ZmbvEncContext *c, uint8_t
*src, int stride,
static int zmbv_me(ZmbvEncContext *c, uint8_t *src, int sstride, uint8_t *prev,
int pstride, int x, int y, int *mx, int *my, int *xored)
{
-int dx, dy, tx, ty, tv, bv, bw, bh;
+int dx, dy, txored, tv, bv, bw, bh;
+int mx0, my0;
-*mx = *my = 0;
+mx0 = *mx;
+my0 = *my;
bw = FFMIN(ZMBV_BLOCK, c->avctx->width - x);
bh = FFMIN(ZMBV_BLOCK, c->avctx->height - y);
+
+/* Try (0,0) */
bv = block_cmp(c, src, sstride, prev, pstride, bw, bh, xored);
+*mx = *my = 0;
if(!bv) return 0;
-for(ty = FFMAX(y - c->range, 0); ty < FFMIN(y + c->range, c->avctx->height
- bh); ty++){
-for(tx = FFMAX(x - c->range, 0); tx < FFMIN(x + c->range,
c->avctx->width - bw); tx++){
-if(tx == x && ty == y) continue; // we already tested this block
-dx = tx - x;
-dy = ty - y;
-tv = block_cmp(c, src, sstride, prev + dx + dy * pstride, pstride,
bw, bh, xored);
+
+/* Try previous block's MV (if not 0,0) */
+if (mx0 || my0){
+tv = block_cmp(c, src, sstride, prev + mx0 + my0 * pstride, pstride,
bw, bh, );
+if(tv < bv){
+bv = tv;
+*mx = mx0;
+*my = my0;
+*xored = txored;
+if(!bv) return 0;
+}
+}
+
+/* Try other MVs from top-to-bottom, left-to-right */
+for(dy = -c->lrange; dy <= c->urange; dy++){
+for(dx = -c->lrange; dx <= c->urange; dx++){
+if(!dx && !dy) continue; // we already tested this block
+if(dx == mx0 && dy == my0) continue; // this one too
+tv = block_cmp(c, src, sstride, prev + dx + dy * pstride, pstride,
bw, bh, );
if(tv < bv){
bv = tv;
*mx = dx;
*my = dy;
+ *xored = txored;
if(!bv) return 0;
}
}
@@ -181,7 +199,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
int x, y, bh2, bw2, xored;
uint8_t *tsrc, *tprev;
uint8_t *mv;
-int mx, my;
+int mx = 0, my = 0;
bw = (avctx->width + ZMBV_BLOCK - 1) / ZMBV_BLOCK;
bh = (avctx->height + ZMBV_BLOCK - 1) / ZMBV_BLOCK;
@@ -269,7 +287,7 @@ static av_cold int encode_end(AVCodecContext *avctx)
av_freep(>work_buf);
deflateEnd(>zstream);
-av_freep(>prev);
+av_freep(>prev_buf);
return 0;
}
@@ -283,6 +301,7 @@ static av_cold int encode_init(AVCodecContext *avctx)
int zret; // Zlib return code
int i;
int lvl = 9;
+int prev_size, prev_offset;
/* Entropy-based score tables for comparing blocks.
* Suitable for blocks up to (ZMBV_BLOCK * ZMBV_BLOCK) bytes.
@@ -295,9 +314,13 @@ static av_cold int encode_init(AVCodecContext *avctx)
c->curfrm = 0;
c->keyint = avctx->keyint_min;
-c->range = 8;
-if(avctx->me_range > 0)
-c->range = FFMIN(avctx->me_range, 127);
+
+/* Motion estimation range: maximum distance is -64..63 */
+c->lrange = c->urange = 8;
+if(avctx->me_range > 0){
+c->lrange = FFMIN(avctx->me_range, 64);
+c->urange = FFMIN(avctx->me_range, 63);
+}
if(avctx->compression_level >= 0)
lvl = avctx->compression_level;
@@ -323,11 +346,16 @@ static av_cold int encode_init(AVCodecContext *avctx)
av_log(avctx, AV_LOG_ERROR, "Can't allocate compression buffer.\n");
return AVERROR(ENOMEM);
}
-c->pstride = FFALIGN(avctx->width, 16);
-if (!(c->prev = av_malloc(c->pstride * avctx->height))) {
+
+/* Allocate prev buffer - leave border around
Re: [FFmpeg-devel] [PATCH 1/4] zmbvenc: don't sum the entropy when blocks are equal
On Sun, 20 Jan 2019 at 15:16, Tomas Härdin wrote: > > Hi. Just to say, I tried setting up additional score_tabs for the > > bottom/right partial blocks. Unfortunately, after implementing it and > > ironing out the bugs, the new score tables actually caused a slight > > increase in file size! > > After a little informal investigation with a couple of videos, my > findings > > were that increasing the divisor - '(i / (double)(ZMBV_BLOCK * > ZMBV_BLOCK * > > some_factor))' - would generally lead to slight compression improvements. > > Given the score table is still "valid" for smaller blocks, and given the > > extra complexity of adding the score tables, plus the fact that it may > > generally hurt the compression, I'm inclined to leave it with the one > score > > table. But there may be potential to improve the current compression > > method in future, by somehow tuning the divisor for better results > > generally. > > Huh, that's strange. Sounds like something that warrants a comment in > the code. I also see we have an answer do Carl's question: you're still > experimenting with this :) I think we can hold off on pushing anything > until you feel happy with the result > Hi. Sorry, I had missed Carl's question. Regrettably my work on this has been slow since my initial patch submissions, but I think I'm close to submitting some final changes. Thanks for your patience so far. I have recently made two helpful realisations about the above score table problem: 1. The entropy calculation in block_cmp() omits the score of histogram[0] from the final sum. It's tempting to do this to bias the scores in favour of 0-bytes, but in reality, blocks with a majority of 0 (or any other byte) will already be naturally favoured by the entropy score anyway, and this bias will fail to penalise blocks with an "average" (i.e. high entropy) number of 0's in them. The exact implications are difficult to ponder, but in practical terms this error does tend to produce worse results in the video compression. Not massively so, but it's still noticeable. (Math is a harsh mistress, and will often look unkindly on such flagrant attempts to circumvent her laws.. :) 2. As long as the blocks being compared are the same size as each other, the entropy-based score comparisons (without the above bias) are unaffected by the base of the log, or the divisor used. (Mathematically, if 'a+b=c+d', then if 'a*log(a) + b*log(b) < c*log(c) + d*log(d)' then it is also true that 'a*log(a/N) + b*log(b/N) < c*log(c/N) + d*log(d/N)'. If that doesn't make sense, it helps to note that 'log(a/N) = log(a) - log(N)', so the log(N)'s cancel out.) This means that we can use a single score table for all block sizes! It only needs to be big enough for the largest block size, then it produces optimal scores for all blocks up to that size. It does mean that partial blocks with uniform bytes won't have a score of 0, but this property of entropy is not actually important when scoring the blocks. (Overall, this is a significant relief, because my attempts at multiple score tables resulted in a lot of tricky decisions about how best to keep the code DRY and minimise complexity, and I wasn't really happy with my solution.) I am planning to organise my submission into two patches. I intend for one to focus on problems/inefficiencies in block comparisons/score tables. The other will be focused on motion estimation, i.e. the range of MV's that are compared, and to a very small extent, the order they're compared in. This avoids the overhead of patching each individual issue separately, but it will provide a logical split between the two main areas of change. Kind regards, Matthew ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] zmbvenc: don't sum the entropy when blocks are equal
On Tue, 25 Dec 2018 at 09:35, Tomas Härdin wrote: > lör 2018-12-22 klockan 15:32 + skrev Matthew Fearnley: > > > > > > > > > Note that bw,bh aren't guaranteed to equal ZMBV_BLOCK, so > `histogram[0] == > > > > bw*bh` would have to be used to guard against those (literal) edge > cases. > > > > > > Right, yes. But if we have block sizes other than ZMBV_BLOCK^2 then we > > > need score tables for those sizes too. > > > > I’ve thought about that a bit. I wondered if it would be worth it given: > > - the extra code, memory and logic needed > > If you have a huge amount of DOS captures to optimize then it might be > worth it, else probably questionable > > > - it would only improve the edge blocks > > I imagine large blocks would be good for scenes with mostly global > motion. You cut down on the number of MVs and thus the amount of data > zlib has to compress, if the block size is a good fit. > > > - the existing score table isn’t catastrophically bad for short blocks, > and would still favour blocks with more common pixels. > > > > It would be better from a correctness perspective though, and effects on > running time should be negligible. > > Good point. There's also no telling whether the current model is > actually an accurate prediction of how zlib behaves :) > > Hi. Just to say, I tried setting up additional score_tabs for the bottom/right partial blocks. Unfortunately, after implementing it and ironing out the bugs, the new score tables actually caused a slight increase in file size! After a little informal investigation with a couple of videos, my findings were that increasing the divisor - '(i / (double)(ZMBV_BLOCK * ZMBV_BLOCK * some_factor))' - would generally lead to slight compression improvements. Given the score table is still "valid" for smaller blocks, and given the extra complexity of adding the score tables, plus the fact that it may generally hurt the compression, I'm inclined to leave it with the one score table. But there may be potential to improve the current compression method in future, by somehow tuning the divisor for better results generally. Matthew ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them
> On 18 Jan 2019, at 23:00, Michael Niedermayer wrote:
>
> Fixes: Timeout
> Fixes:
> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>
> Before:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 15423 ms
> After:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 190 ms
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/rscc.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> index 7921f149ed..3868c1cb1b 100644
> --- a/libavcodec/rscc.c
> +++ b/libavcodec/rscc.c
> @@ -64,6 +64,7 @@ typedef struct RsccContext {
> /* zlib interaction */
> uint8_t *inflated_buf;
> uLongf inflated_size;
> +int valid_pixels
> } RsccContext;
>
> static av_cold int rscc_init(AVCodecContext *avctx)
> @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx, void
> *data,
> memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
> }
>
> -*got_frame = 1;
> +// We only return a picture when too little is undameged, this avoids
> copying nearly broken frames around
Hi,
FWIW, I think “too little is undameged” should say: “enough of it is undamaged”
(i.e. invert the logic, fix the typo).
> +if (ctx->valid_pixels < ctx->inflated_size)
> +ctx->valid_pixels += pixel_size;
> +if (ctx->valid_pixels >= ctx->inflated_size * (100 -
> avctx->discard_damaged_percentage) / 100)
> +*got_frame = 1;
>
> ret = avpkt->size;
> end:
> --
> 2.20.1
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/4] zmbvenc: use unsigned values for score calculations
> On 22 Dec 2018, at 12:15, Tomas Härdin wrote: > > tor 2018-12-20 klockan 17:48 +0000 skrev Matthew Fearnley: >>>>> On Thu, 20 Dec 2018 at 16:30, Tomas Härdin wrote: >>> >>> Trivial enough. You could probably roll many or even all of these >>> patches together >> >> Thanks for your feedback. I was reluctant to submit so many patches, but I >> worried at the time that combining them would require an overly long commit >> message. Maybe I'm being too verbose though... >> What's the best way to submit a combined patch - from a Patchwork/mailing >> list perspective? > > How large a patch should be is somewhat arbitrary. I tend to try to > keep each patch related to a single "concept". And of course separating > cosmetic and functional changes > > It's mostly the having to have PATCH 3/4 before PATCH 1/4 prompting > this suggestion. Or maybe rolling them together If I roll the patches together (and include the additional suggestions in PATCH 1), would it be best to just email a new patch, and have these closed? Still struggling with general procedure here a bit... Thanks for bearing with me. Matthew ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/4] zmbvenc: Prevent memory/math overflows in block_cmp()
> On 20 Dec 2018, at 16:30, Tomas Härdin wrote:
>
> ons 2018-12-19 klockan 22:00 + skrev matthew.w.fearn...@gmail.com:
>>> From: Matthew Fearnley
>>
>> score_tab[] was only declared/initialised for elements 0..255, but with
>> block sizes set to 16*16, it was possible to reach 256.
>>
>> This limit could also be overflowed in the histogram, because it was
>> declared with a uint8_t type.
>>
>> This can be fixed, and also allow different ZMBV_BLOCK sizes, by making
>> score_tab[] with (ZMBV_BLOCK*ZMBV_BLOCK+1) elements, and declaring
>> histogram[] to use a uint16_t type.
>>
>> Note: the maximum block size possible for PAL8 is 255*255 bytes, which is
>> close
>> to the uint16_t limit. To support full-colour pixel formats, a uint32_t
>> could
>> potentially be required.
>
> So a potential future encoder improvement would be to try different
> block sizes? Could be a fun project for someone, like a GSoC
> qualification task
Yeah. The header allows for block dimensions up to 255, and the width and
height can be different from each other.
They can technically be changed in each keyframe header, but I’ve not tested
how the decoder handles that..
>> @@ -69,7 +69,7 @@ static inline int block_cmp(ZmbvEncContext *c, uint8_t
>> *src, int stride,
>> {
>> int sum = 0;
>> int i, j;
>> -uint8_t histogram[256] = {0};
>> +uint16_t histogram[256] = {0};
>>
>> /* build frequency histogram of byte values for src[] ^ src2[] */
>> *xored = 0;
>> @@ -285,7 +285,9 @@ static av_cold int encode_init(AVCodecContext *avctx)
>> int i;
>> int lvl = 9;
>>
>> -for(i=1; i<256; i++)
>> +/* entropy score table for block_cmp() */
>> +c->score_tab[0] = 0;
>> +for(i = 1; i <= ZMBV_BLOCK * ZMBV_BLOCK; i++)
>> c->score_tab[i] = -i * log2(i / (double)(ZMBV_BLOCK * ZMBV_BLOCK))
>> * 256;
>
> It strikes me that due to the uint8_t overflow the old code actually
> worked correctly, but only incidentally..
It’s almost ingenious, the way it manages that. I almost didn’t want to change
it..
> Looks good!
Thanks :)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] zmbvenc: don't sum the entropy when blocks are equal
> On 22 Dec 2018, at 12:11, Tomas Härdin wrote: > > tor 2018-12-20 klockan 17:46 +0000 skrev Matthew Fearnley: >>>> On Thu, 20 Dec 2018 at 16:30, Tomas Härdin wrote: >>> >>> I have a feeling this could be sped up further by just doing *xored = >>> histogram[0] == ZMBV_BLOCK*ZMBV_BLOCK after the loops, if [PATCH 3/4] >>> is applied before this. Computing both histogram and xored in the loop >>> seems pointless. >>> >> >> You're right, that speedup didn't occur to me. It makes the logic a bit >> more tenuous, but it would be more efficient. > > Eh, I wouldn't really call "we have xored data to output if and only if > number of zeroes < number of pixel" fairly easy to grasp. A comment > might be good tho Agreed. Just have to get the patch order sorted out now somehow. >> Note that bw,bh aren't guaranteed to equal ZMBV_BLOCK, so `histogram[0] == >> bw*bh` would have to be used to guard against those (literal) edge cases. > > Right, yes. But if we have block sizes other than ZMBV_BLOCK^2 then we > need score tables for those sizes too. I’ve thought about that a bit. I wondered if it would be worth it given: - the extra code, memory and logic needed - it would only improve the edge blocks - the existing score table isn’t catastrophically bad for short blocks, and would still favour blocks with more common pixels. It would be better from a correctness perspective though, and effects on running time should be negligible. I can work on a patch, see how it looks. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/4] zmbvenc: use unsigned values for score calculations
On Thu, 20 Dec 2018 at 16:30, Tomas Härdin wrote: > Trivial enough. You could probably roll many or even all of these > patches together > Thanks for your feedback. I was reluctant to submit so many patches, but I worried at the time that combining them would require an overly long commit message. Maybe I'm being too verbose though... What's the best way to submit a combined patch - from a Patchwork/mailing list perspective? Thanks again for your reviews. I'm happy to make any changes necessary for them to be committed. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] zmbvenc: don't sum the entropy when blocks are equal
On Thu, 20 Dec 2018 at 16:30, Tomas Härdin wrote: > I have a feeling this could be sped up further by just doing *xored = > histogram[0] == ZMBV_BLOCK*ZMBV_BLOCK after the loops, if [PATCH 3/4] > is applied before this. Computing both histogram and xored in the loop > seems pointless. > You're right, that speedup didn't occur to me. It makes the logic a bit more tenuous, but it would be more efficient. Note that bw,bh aren't guaranteed to equal ZMBV_BLOCK, so `histogram[0] == bw*bh` would have to be used to guard against those (literal) edge cases. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavcodec/zmbvenc.c: don't allow motion estimation out of range.
Hi, Just to say, after further reviewing the logic in zmbv_me() and block_cmp(), I think it all tenuously holds together, with no possibilities of incorrect xored values or undefined behaviour. But it is quite fragile. I’m planning some patches to strengthen/clarify the logic, and to make it more resilient to future changes, but I think this patch is good to go, as it is now. Kind regards Matthew > On 15 Dec 2018, at 20:54, Matthew Fearnley > wrote: > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavcodec/zmbvenc.c: don't allow motion estimation out of range.
On Sun, 9 Dec 2018 at 16:27, Michael Niedermayer
wrote:
> On Sat, Dec 08, 2018 at 02:53:44PM +0100, Tomas Härdin wrote:
> > lör 2018-12-08 klockan 00:29 +0000 skrev Matthew Fearnley:
> > > Hi Tomas, thanks for looking through my patch.
> > >
> > > > > Practically, this patch fixes graphical glitches e.g. when
> reencoding
> > >
> > > the
> > > > > Commander Keen sample video with me_range 65 or higher:
> > > > >
> > > > > ffmpeg -i keen4e_000.avi -c:v zmbv -me_range 65 keen4e_me65.avi
> > > > I'd expect this problem to pop up with -me_range 64 too, no?
> > >
> > > I initially thought this would be the case, but taking the tx loop and
> > > removing the edge constraints:
> > >
> > > for(tx = x - c->range; tx < x + c->range; tx++){
> > > ...
> > > dx = tx - x;
> > >
> > > The effective range is (-c->range) <= dx < (c->range), meaning when
> > > c->range = me_range = 64, the dx value ranges from -64 to 63, which
> happens
> > > to be exactly in bounds.
> > > So I could have just capped me_range to 64, and that would have fixed
> the
> > > bug...
> > >
> > > But more generally, I've concluded the '<' sign is a mistake, not just
> > > because of the general asymmetry, but because of the way it prevents
> tx,ty
> > > reaching the bottom/right edges.
> > > In practice it means, for example, that if the screen pans left to
> right,
> > > the bottom edge will have to match against blocks elsewhere in the
> image.
> > >
> > > > I went over the patch, and it looks fine. But what's up with the
> xored
> > > > logic? It seems like it would compute xored always from the bottom-
> > > > right-most MV. The loop in zmbv_me() should probably have a temporary
> > > > xored and only output to *xored in if(tv < bv)..
> > >
> > > Hmm, you're right. In most cases, the code actually works anyway -
> because
> > > when *xored==0, the block entropy returned by block_cmp() is supposed
> to be
> > > 0 anyway, so it still finishes then.
> > > But... I've realised there are some exceptions to this:
> > > - the entropy calculations in block_cmp() use a lookup table
> > > (score_tab[256]), which assumes each block has 16*16 pixels. This will
> > > only be valid when the dimensions are a multiple of 16, otherwise the
> > > bottom/right edge blocks may be smaller.
> > > - I've just realised the lookup table only goes up to 255. If all
> 16*16
> > > xored pixels are the same value, it will hit the 256th entry! (I'm
> > > surprised valgrind hasn't found this..?)
> >
> > Static analysis would've caught this, which is something I've harped on
> > previously on this list (http://ffmpeg.org/pipermail/ffmpeg-devel/2018-
> > June/231598.html)
> >
> > > All that said, *xored==0 is actually the most desirable outcome
> because it
> > > means the block doesn't need to be output.
> > > So if *xored is 0, the best thing to do is probably to finish
> immediately
> > > (making sure *mx,*my are set), without processing any more MVs.
> > > And then it doesn't matter (from a correctness point of view, at
> least) if
> > > block_cmp() gives bad entropy results, as long as *xored is set
> correctly.
> >
> > Oh yeah, that's right. It might also be a good idea to try the MV of
> > the previous block first for the next block, since consecutive blocks
> > are likely to have the same MV. Even fancier would be to gather
> > statistics from the previous frame, and try MVs in order of decreasing
> > popularity. A threshold might also be useful, like "accept any MV that
> > results in no more than N bits entropy"
>
I have a feeling that this isn't a strategy that helps much in the worst
case. If the upper half of a block contains random data, there's still
potential for compression, but you wouldn't get a good enough sense of the
entropy until you've run block_cmp more than half-way for every MV.
Could still be worth it though. You could probably presume blocks like
that would be infrequent in the kind of videos ZMBV is designed for, and
just write them off with a suboptimal MV.
(This is presuming we don't go along the path suggested by Michael below.)
looking at existing fancy motion estimation methods, for example variants of
> predictive zonal searches. Might safe some time from redesigning a ME
> method
> from scratch
> also maybe something from libavcodec/
Re: [FFmpeg-devel] [PATCH] libavcodec/zmbvenc.c: don't allow motion estimation out of range.
Hi Tomas, thanks for looking through my patch.
> > Practically, this patch fixes graphical glitches e.g. when reencoding
the
> > Commander Keen sample video with me_range 65 or higher:
> >
> > ffmpeg -i keen4e_000.avi -c:v zmbv -me_range 65 keen4e_me65.avi
> I'd expect this problem to pop up with -me_range 64 too, no?
I initially thought this would be the case, but taking the tx loop and
removing the edge constraints:
for(tx = x - c->range; tx < x + c->range; tx++){
...
dx = tx - x;
The effective range is (-c->range) <= dx < (c->range), meaning when
c->range = me_range = 64, the dx value ranges from -64 to 63, which happens
to be exactly in bounds.
So I could have just capped me_range to 64, and that would have fixed the
bug...
But more generally, I've concluded the '<' sign is a mistake, not just
because of the general asymmetry, but because of the way it prevents tx,ty
reaching the bottom/right edges.
In practice it means, for example, that if the screen pans left to right,
the bottom edge will have to match against blocks elsewhere in the image.
> I went over the patch, and it looks fine. But what's up with the xored
> logic? It seems like it would compute xored always from the bottom-
> right-most MV. The loop in zmbv_me() should probably have a temporary
> xored and only output to *xored in if(tv < bv)..
Hmm, you're right. In most cases, the code actually works anyway - because
when *xored==0, the block entropy returned by block_cmp() is supposed to be
0 anyway, so it still finishes then.
But... I've realised there are some exceptions to this:
- the entropy calculations in block_cmp() use a lookup table
(score_tab[256]), which assumes each block has 16*16 pixels. This will
only be valid when the dimensions are a multiple of 16, otherwise the
bottom/right edge blocks may be smaller.
- I've just realised the lookup table only goes up to 255. If all 16*16
xored pixels are the same value, it will hit the 256th entry! (I'm
surprised valgrind hasn't found this..?)
All that said, *xored==0 is actually the most desirable outcome because it
means the block doesn't need to be output.
So if *xored is 0, the best thing to do is probably to finish immediately
(making sure *mx,*my are set), without processing any more MVs.
And then it doesn't matter (from a correctness point of view, at least) if
block_cmp() gives bad entropy results, as long as *xored is set correctly.
Note: the code currently exits on bv==0. It's a very good result, but it
does not always imply the most desirable case, because it will happen if
the xored block contains all 42's (etc), not just all 0's.
It's obviously highly compressible, but it would still be better if the
block could be omitted entirely. Maybe it's an acceptable time/space
tradeoff though. I don't know...
So I guess there are at least two more fixes that need to be made:
- score_tab[] should have 257 elements (well actually, it should have
ZMBV_BLOCK*ZMBV_BLOCK+1 elements.)
- zmbv_me() should set *mx,*my and finish early, if block_cmp() (either at
the start or in the loop) sets *xored to 0.
Matthew
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
