[PATCH]Lagarith decoder.
Here are the latest Lagarith patches. The only changes to the rangecoder from the previous patch are cosmetic (K&R formatting). The decoder includes Loren's floating point emulation. Thanks to everyone, and especially Loren and Reimar, for all their help. -- -Nathan Caldwell
On Tue, Sep 22, 2009 at 11:54:03PM -0600, Nathan Caldwell wrote:
Here are the latest Lagarith patches. The only changes to the rangecoder from the previous patch are cosmetic (K&R formatting). The decoder includes Loren's floating point emulation. Thanks to everyone, and especially Loren and Reimar, for all their help.
Any further comments on this? Or should it just be applied and we see what comes of it? I'm asking because recently I saw quite a few people asking about it, so there is a good chance it will receive testing if applied. I'll note that I find it a bit unfortunate that the codec does not include a checksum which would make it easy to verify decoding.
On Sat, Oct 10, 2009 at 7:59 AM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Tue, Sep 22, 2009 at 11:54:03PM -0600, Nathan Caldwell wrote:
Here are the latest Lagarith patches. The only changes to the rangecoder from the previous patch are cosmetic (K&R formatting). The decoder includes Loren's floating point emulation. Thanks to everyone, and especially Loren and Reimar, for all their help.
Any further comments on this? Or should it just be applied and we see what comes of it?
I have no problem with committing just YV12, I did it first because I figured it was the most useful. I can strip out the (currently) unused code for RGB if desired. I do plan on adding support for other colorspaces, not sure when I'll get around to it however. -- -Nathan Caldwell
On Tue, Sep 22, 2009 at 11:54:03PM -0600, Nathan Caldwell wrote:
Here are the latest Lagarith patches. The only changes to the rangecoder from the previous patch are cosmetic (K&R formatting). The decoder includes Loren's floating point emulation. Thanks to everyone, and especially Loren and Reimar, for all their help.
[...]
+typedef struct lag_rac { + AVCodecContext *avctx; + unsigned low; + unsigned range; + unsigned scale; /*!< Number of bits of precision in range. */ + unsigned hash_shift; /*!< Number of bits to shift to calculate hash for radix search. */ + + uint8_t *bytestream_start; /*!< Start of input bytestream. */ + uint8_t *bytestream; /*!< Current position in input bytestream. */ + uint8_t *bytestream_end; /*!< End position of input bytestream. */ +
+ int prob[258]; /*!< Table of cumulative probability for each symbol. */
this is filled with uint32_t giving nice negative probs [...]
+/* compute the 52bit mantissa of 1/(double)denom */ +static uint64_t softfloat_reciprocal(uint32_t denom)
comment not doxygen compatible and this really needs some explanation of why it is used instead of 1.0/denom in the doxy [...]
+static uint32_t lag_decode_prob(GetBitContext *gb) +{ + static const uint8_t series[] = { 1, 2, 3, 5, 8, 13, 21, 34 };
where is series[7] accessed?
+ int i; + int bit = 0; + int bits = 0; + int prevbit = 0; + unsigned value = 1; + + for (i = 0; i < 7; i++) { + if (prevbit && bit) + break; + prevbit = bit; + bit = get_bits1(gb); + if (bit && !prevbit) + bits += series[i]; + } + bits--;
+ if (bits <= 0) + return 0; + if (bits > 31) { + /* This is most likely an error, just use the first 32 bits */ + bits = 31; + }
are these not error conditions that warrent a error message and failure?
+ + value = get_bits_long(gb, bits); + value |= 1 << bits; + + return value - 1; +} + +static int lag_read_prob_header(lag_rac *rac, GetBitContext *gb) +{ + int i, j; + int prob = 0; + int scale_factor = 0; + unsigned cumul_prob = 0; + unsigned cumulative_target = 1; + unsigned scaled_cumul_prob = 0; + + rac->prob[0] = 0; + rac->prob[257] = UINT_MAX; + /* Read probabilities from bitstream */ + for (i = 1; i < 257; i++) { + rac->prob[i] = lag_decode_prob(gb);
+ cumul_prob += rac->prob[i];
can overflow
+ if (!rac->prob[i]) {
+ prob = lag_decode_prob(gb); + if (i + prob >= 258) + prob = 257 - i;
integer overflow, segfault below and isnt this check supposed to be an error condition instead of silent cliping?
+ for (j = 0; j < prob; j++) + rac->prob[++i] = 0;
[...]
+static inline int add_left_prediction(uint8_t *dst, uint8_t *src, int w, + int acc) +{ + int i; + + for (i = 0; i < w - 1; i++) { + acc += src[i]; + dst[i] = acc; + i++; + acc += src[i]; + dst[i] = acc; + } + + for (; i < w; i++) { + acc += src[i]; + dst[i] = acc; + } + + return acc; +}
code duplication from dsputil [...]
+static int lag_decode_line(LagarithContext *l, lag_rac *rac, + uint8_t *dst, int width, int stride, + int step, int esc_count) +{ + int i = 0; + int ret = 0; + + /* Output any zeros remaining from the previous run */ + if (l->zeros_rem) { + int count = FFMIN(l->zeros_rem, width); + lag_memset(dst + i * step, 0, count, step); + i += count; + l->zeros_rem -= count; + } + + while (i < width) { + + dst[i * step] = lag_get_rac(rac); + ret++; +
+ if (dst[i * step]) + l->zeros = 0; + else + l->zeros++;
maybe l->zeros += !!dst[i * step]; is faster also getting rid of *step and doing +=step might be faster and then variables like zeros could be tried to be moved onto the stack
+ + i++; + if (esc_count && (l->zeros == esc_count)) {
the esc_count check can likely be removed by changing esc_count=0 -> -1
+ int count; + int index = lag_get_rac(rac); + ret++; + + l->zeros = 0; +
+ l->zeros_rem = + count = lag_calc_zero_run(index); + + if (i + count > width) + count = width - i; + if (!count) + continue; + + lag_memset(dst + i * step, 0, count, step); + + i += count; + l->zeros_rem -= count;
l->zeros_rem = lag_calc_zero_run(index); goto handle_zeros; (above in that if before the while)
+ } + } + return ret; +} +
+static int lag_decode_zero_run_line(LagarithContext *l, uint8_t *dst, + const uint8_t *src, int width, + int step, int esc_count) +{ + int i = 0; + int count; + uint8_t zero_run = 1; + const uint8_t *start = src; + uint8_t mask1 = -(esc_count < 2); + uint8_t mask2 = -(esc_count < 3); + uint8_t *end = dst + (width - 2) * step; + + if (l->zeros_rem) { + count = FFMIN(l->zeros_rem, width); + lag_memset(dst, 0, count, step); + l->zeros_rem -= count; + dst += count; + } + + while (dst < end) { + i = 0; + while (!zero_run && dst + i * step < end) { + i++; + zero_run = + (src[i] | (src[i + 1] & mask1) | (src[i + 2] & mask2)); + } + if (!zero_run) { + i += esc_count; + lag_memcpy(dst, src, i, step); + dst += i; + l->zeros_rem = + count = lag_calc_zero_run(src[i]); + + if (dst + l->zeros_rem * step > end) + count = (end - dst) / step; + + lag_memset(dst, 0, count, step); + dst += count; + src += i + 1; + l->zeros_rem -= count; + } else { + lag_memcpy(dst, src, i, step); + src += i; + } + } + return start - src; +}
am i missing something or is half of that code unreachable and zero_run always 1? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In fact, the RIAA has been known to suggest that students drop out of college or go to community college in order to be able to afford settlements. -- The RIAA
On Tue, Oct 13, 2009 at 3:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Sep 22, 2009 at 11:54:03PM -0600, Nathan Caldwell wrote:
Here are the latest Lagarith patches. The only changes to the rangecoder from the previous patch are cosmetic (K&R formatting). The decoder includes Loren's floating point emulation. Thanks to everyone, and especially Loren and Reimar, for all their help.
+ int prob[258]; /*!< Table of cumulative probability for each symbol. */
this is filled with uint32_t giving nice negative probs
Fixed.
+/* compute the 52bit mantissa of 1/(double)denom */ +static uint64_t softfloat_reciprocal(uint32_t denom)
comment not doxygen compatible and this really needs some explanation of why it is used instead of 1.0/denom in the doxy
Updated. It would be nice if Loren could comment on these descriptions.
+static uint32_t lag_decode_prob(GetBitContext *gb) +{ + static const uint8_t series[] = { 1, 2, 3, 5, 8, 13, 21, 34 };
where is series[7] accessed?
It isn't. The table was copied from reference. Removed.
+ if (bits <= 0) + return 0; + if (bits > 31) { + /* This is most likely an error, just use the first 32 bits */ + bits = 31; + }
are these not error conditions that warrent a error message and failure?
In the case of bits==0, no. Any other case (bits < 0 or bits >31), yes. This should be fixed now.
+ + value = get_bits_long(gb, bits); + value |= 1 << bits; + + return value - 1; +} + +static int lag_read_prob_header(lag_rac *rac, GetBitContext *gb) +{ + int i, j; + int prob = 0; + int scale_factor = 0; + unsigned cumul_prob = 0; + unsigned cumulative_target = 1; + unsigned scaled_cumul_prob = 0; + + rac->prob[0] = 0; + rac->prob[257] = UINT_MAX; + /* Read probabilities from bitstream */ + for (i = 1; i < 257; i++) { + rac->prob[i] = lag_decode_prob(gb);
+ cumul_prob += rac->prob[i];
can overflow
In this case the reference decoder would also overflow. Would you prefer to match the reference, or not overflow?
+ if (!rac->prob[i]) {
+ prob = lag_decode_prob(gb); + if (i + prob >= 258) + prob = 257 - i;
integer overflow, segfault below
Fixed.
and isnt this check supposed to be an error condition instead of silent cliping?
In a perfect world, yes. Again, just matching the behavior of the reference decoder.
+static inline int add_left_prediction(uint8_t *dst, uint8_t *src, int w, + int acc)
code duplication from dsputil
I submitted this patch before the one that moved the huffyuv prediction functions to dsputil. Fixed.
[...]
+ if (dst[i * step]) + l->zeros = 0; + else + l->zeros++;
maybe l->zeros += !!dst[i * step]; is faster
This won't work because we only want to count consecutive runs of zeros. l->zeros needs to be reset to 0 at each non-zero byte.
also getting rid of *step and doing +=step might be faster and then variables like zeros could be tried to be moved onto the stack
This is what I originally had, however it introduces too many divisions in the calculation of the zero runs.
+ + i++; + if (esc_count && (l->zeros == esc_count)) {
the esc_count check can likely be removed by changing esc_count=0 -> -1
Done.
+ l->zeros_rem = + count = lag_calc_zero_run(index); + + if (i + count > width) + count = width - i; + if (!count) + continue; + + lag_memset(dst + i * step, 0, count, step); + + i += count; + l->zeros_rem -= count;
l->zeros_rem = lag_calc_zero_run(index); goto handle_zeros; (above in that if before the while)
Done.
+ } + } + return ret; +} +
+static int lag_decode_zero_run_line(LagarithContext *l, uint8_t *dst, + const uint8_t *src, int width, + int step, int esc_count)
am i missing something or is half of that code unreachable and zero_run always 1?
Oops, you are correct. A few bugs in there. -- -Nathan Caldwell
On Thu, 15 Oct 2009, Nathan Caldwell wrote:
On Tue, Oct 13, 2009 at 3:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
+/* compute the 52bit mantissa of 1/(double)denom */ +static uint64_t softfloat_reciprocal(uint32_t denom)
comment not doxygen compatible and this really needs some explanation of why it is used instead of 1.0/denom in the doxy
Updated. It would be nice if Loren could comment on these descriptions.
+ This is used because it is more portable across architectures and + toolchains than using floats.
I wouldn't say that explains anything. How about: This crazy format uses floats in an entropy coder, thus we have to match x86's rounding exactly, thus ordinary floats aren't portable enough. --Loren Merritt
On Fri, Oct 16, 2009 at 6:46 AM, Loren Merritt <lorenm at u.washington.edu> wrote:
On Thu, 15 Oct 2009, Nathan Caldwell wrote:
On Tue, Oct 13, 2009 at 3:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
+/* compute the 52bit mantissa of 1/(double)denom */ +static uint64_t softfloat_reciprocal(uint32_t denom)
comment not doxygen compatible and this really needs some explanation of why it is used instead of 1.0/denom in the doxy
Updated. It would be nice if Loren could comment on these descriptions.
+ This is used because it is more portable across architectures and + toolchains than using floats.
I wouldn't say that explains anything. How about:
This crazy format uses floats in an entropy coder, thus we have to match x86's rounding exactly, thus ordinary floats aren't portable enough.
Haha, I like it. Updated locally. -- -Nathan Caldwell
On Thu, Oct 15, 2009 at 08:42:54PM -0600, Nathan Caldwell wrote:
--- /dev/null +++ b/libavcodec/lagarithrac.h @@ -0,0 +1,112 @@ + +#endif /* AVCODEC_LAGARITHRAC_H */
Please use just one space, like we do everywhere else.
--- /dev/null +++ b/libavcodec/lagarith.c @@ -0,0 +1,543 @@ + +/** + * @file libavcodec/lagarith.c + * Lagarith lossless decoder + * @author Nathan Caldwell + * + */
pointless empty line
+enum LagarithFrameType { + FRAME_RAW = 1, /*!< Uncompressed */ + FRAME_U_RGB24 = 2, /*!< Unaligned RGB24 */ + FRAME_ARITH_YUY2 = 3, /*!< Arith coded YUY2 */ + FRAME_ARITH_RGB24 = 4, /*!< Arith coded RGB24 */ + FRAME_SOLID_GRAY = 5, /*!< Solid grayscale color frame */ + FRAME_SOLID_COLOR = 6, /*!< Solid non-grayscale color frame */ + FRAME_OLD_ARITH_RGB = 7, /*!< Obsolete arithmetic coded RGB (Maintained for backwards compatibility) */ + FRAME_ARITH_RGBA = 8, /*!< Arithmetic coded RGBA */ + FRAME_SOLID_RGBA = 9, /*!< Solid RGBA color frame */ + FRAME_ARITH_YV12 = 10, /*!< Arithmetic coded YV12 */ + FRAME_REDUCED_RES = 11, /*!< Reduced resolution frame */
Sometimes you say "arith coded", sometimes arithmetic coded", please use the latter everywhere. Also, there is no need to capitalize these non-sentences. You don't do it in other places.
+ int zeros; /*!< number of consecutave zero bytes encountered */
consecutIve
+ read += + lag_decode_line(l, &rac, dst + (i * stride), width, + stride, step, esc_count); + + src += + lag_decode_zero_run_line(l, dst + (i * stride), + src, width, step, esc_count);
The function calls on the next line look weird. Diego
On Mon, Oct 19, 2009 at 8:18 AM, Diego Biurrun <diego at biurrun.de> wrote:
On Thu, Oct 15, 2009 at 08:42:54PM -0600, Nathan Caldwell wrote:
--- /dev/null +++ b/libavcodec/lagarithrac.h @@ -0,0 +1,112 @@ + +#endif ? ? ? ? ? ? ? ? ? ? ? ? ?/* AVCODEC_LAGARITHRAC_H */
Please use just one space, like we do everywhere else.
Fixed.
+ * @author Nathan Caldwell + * + */
pointless empty line
Fixed.
+ ? ?FRAME_ARITH_YUY2 ? ?= 3, ? ?/*!< Arith coded YUY2 */ + ? ?FRAME_ARITH_RGB24 ? = 4, ? ?/*!< Arith coded RGB24 */ + ? ?FRAME_SOLID_GRAY ? ?= 5, ? ?/*!< Solid grayscale color frame */ + ? ?FRAME_SOLID_COLOR ? = 6, ? ?/*!< Solid non-grayscale color frame */ + ? ?FRAME_OLD_ARITH_RGB = 7, ? ?/*!< Obsolete arithmetic coded RGB (Maintained for backwards compatibility) */ + ? ?FRAME_ARITH_RGBA ? ?= 8, ? ?/*!< Arithmetic coded RGBA */ + ? ?FRAME_SOLID_RGBA ? ?= 9, ? ?/*!< Solid RGBA color frame */ + ? ?FRAME_ARITH_YV12 ? ?= 10, ? /*!< Arithmetic coded YV12 */ + ? ?FRAME_REDUCED_RES ? = 11, ? /*!< Reduced resolution frame */
Sometimes you say "arith coded", sometimes arithmetic coded", please use the latter everywhere. ?Also, there is no need to capitalize these non-sentences. ?You don't do it in other places.
Fixed.
+ ? ?int zeros; ? ? ? ? ? ? ? ? ?/*!< number of consecutave zero bytes encountered */
consecutIve
Fixed.
+ ? ? ? ? ? ?read += + ? ? ? ? ? ? ? ?lag_decode_line(l, &rac, dst + (i * stride), width, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?stride, step, esc_count); + + ? ? ? ? ? ? ? ?src += + ? ? ? ? ? ? ? ? ? ?lag_decode_zero_run_line(l, dst + (i * stride), + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? src, width, step, esc_count);
The function calls on the next line look weird.
indent's doing. Fixed. Since these are all cosmetic, changes in the next patch. -- -Nathan Caldwell
On Sat, Oct 24, 2009 at 10:54:33PM -0600, Nathan Caldwell wrote:
+ ? ? ? ? ? ?read += + ? ? ? ? ? ? ? ?lag_decode_line(l, &rac, dst + (i * stride), width, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?stride, step, esc_count); + + ? ? ? ? ? ? ? ?src += + ? ? ? ? ? ? ? ? ? ?lag_decode_zero_run_line(l, dst + (i * stride), + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? src, width, step, esc_count);
The function calls on the next line look weird.
indent's doing. Fixed. Since these are all cosmetic, changes in the next patch.
Any chances you can send a patch with the changes you already did? + if (l->low >= range_scaled * l->prob[255]) { + val = 255; + l->range -= range_scaled * l->prob[255]; + } else { [...] + l->range = range_scaled * (l->prob[val + 1] - l->prob[val]); + } + + l->low -= range_scaled * l->prob[val]; Maybe this could be "simplified" to + if (l->low >= range_scaled * l->prob[255]) { + val = 255; + range_upper = l->range; + } else { [...] + range_upper = range_scaled * l->prob[val + 1]; + } + + range_lower = range_scaled * l->prob[val]; + l->range = range_upper - range_lower; + l->low -= range_lower; Assuming the intermediate terms would not overflow. Not useful speed-wise, just maybe a bit clearer on the idea behind the code...
On Sun, Dec 13, 2009 at 09:51:10PM +0100, Reimar D?ffinger wrote:
On Sat, Oct 24, 2009 at 10:54:33PM -0600, Nathan Caldwell wrote:
+ ? ? ? ? ? ?read += + ? ? ? ? ? ? ? ?lag_decode_line(l, &rac, dst + (i * stride), width, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?stride, step, esc_count); + + ? ? ? ? ? ? ? ?src += + ? ? ? ? ? ? ? ? ? ?lag_decode_zero_run_line(l, dst + (i * stride), + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? src, width, step, esc_count);
The function calls on the next line look weird.
indent's doing. Fixed. Since these are all cosmetic, changes in the next patch.
Any chances you can send a patch with the changes you already did?
Does anyone have a version of the patch with the changes that were already done locally? Or can we get this started again? Despite the horribleness of the codec a lot of people are interested...
On Wed, Jan 5, 2011 at 9:15 AM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Sun, Dec 13, 2009 at 09:51:10PM +0100, Reimar D?ffinger wrote:
On Sat, Oct 24, 2009 at 10:54:33PM -0600, Nathan Caldwell wrote:
+ ? ? ? ? ? ?read += + ? ? ? ? ? ? ? ?lag_decode_line(l, &rac, dst + (i * stride), width, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?stride, step, esc_count); + + ? ? ? ? ? ? ? ?src += + ? ? ? ? ? ? ? ? ? ?lag_decode_zero_run_line(l, dst + (i * stride), + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? src, width, step, esc_count);
The function calls on the next line look weird.
indent's doing. Fixed. Since these are all cosmetic, changes in the next patch.
Any chances you can send a patch with the changes you already did?
Does anyone have a version of the patch with the changes that were already done locally? Or can we get this started again? Despite the horribleness of the codec a lot of people are interested...
Hmm, somehow I missed your reply last year. =( Pretty much all my spare free time is taken up by the aac encoder right now. I think I pushed my last changes to http://repo.or.cz/w/FFMpeg-mirror/lagarith.git I've attached rebased patches for the repo I have locally, not sure if these include all the changes. I'll double check my repo at home tonight to see what I have there. -- -Nathan Caldwell
On Wed, Jan 05, 2011 at 03:37:29PM -0700, Nathan Caldwell wrote:
On Wed, Jan 5, 2011 at 9:15 AM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Sun, Dec 13, 2009 at 09:51:10PM +0100, Reimar D?ffinger wrote:
On Sat, Oct 24, 2009 at 10:54:33PM -0600, Nathan Caldwell wrote:
+ ? ? ? ? ? ?read += + ? ? ? ? ? ? ? ?lag_decode_line(l, &rac, dst + (i * stride), width, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?stride, step, esc_count); + + ? ? ? ? ? ? ? ?src += + ? ? ? ? ? ? ? ? ? ?lag_decode_zero_run_line(l, dst + (i * stride), + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? src, width, step, esc_count);
The function calls on the next line look weird.
indent's doing. Fixed. Since these are all cosmetic, changes in the next patch.
Any chances you can send a patch with the changes you already did?
Does anyone have a version of the patch with the changes that were already done locally? Or can we get this started again? Despite the horribleness of the codec a lot of people are interested...
Hmm, somehow I missed your reply last year. =(
Pretty much all my spare free time is taken up by the aac encoder right now.
I suggest we commit this to main svn and continue improvments from there that way its easier for other people to help and its harder to become forgotten and rot If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
On Sat, Jan 8, 2011 at 7:11 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Jan 05, 2011 at 03:37:29PM -0700, Nathan Caldwell wrote:
On Wed, Jan 5, 2011 at 9:15 AM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
On Sun, Dec 13, 2009 at 09:51:10PM +0100, Reimar D?ffinger wrote:
On Sat, Oct 24, 2009 at 10:54:33PM -0600, Nathan Caldwell wrote:
> + ? ? ? ? ? ?read += > + ? ? ? ? ? ? ? ?lag_decode_line(l, &rac, dst + (i * stride), width, > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?stride, step, esc_count); > + > + ? ? ? ? ? ? ? ?src += > + ? ? ? ? ? ? ? ? ? ?lag_decode_zero_run_line(l, dst + (i * stride), > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? src, width, step, esc_count);
The function calls on the next line look weird.
indent's doing. Fixed. Since these are all cosmetic, changes in the next patch.
Any chances you can send a patch with the changes you already did?
Does anyone have a version of the patch with the changes that were already done locally? Or can we get this started again? Despite the horribleness of the codec a lot of people are interested...
Hmm, somehow I missed your reply last year. =(
Pretty much all my spare free time is taken up by the aac encoder right now.
I suggest we commit this to main svn and continue improvments from there that way its easier for other people to help and its harder to become forgotten and rot
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever
This sounds fine to me. I was still getting small differences in one of the chroma channels between the encoder input, and the output of our decoder. They weren't visible, and didn't seem to cause any prediction errors, so I'm not sure if this was due to the format (which I suspect), or if there's a small bug in the decoder somewhere. I never had a chance to check if the reference decoder did the same thing or not. -- -Nathan Caldwell
Nathan Caldwell <saintdev <at> gmail.com> writes:
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever
Since several people stated here Lagarith is broken by definition, what about a warning in decode_init()?
This sounds fine to me.
Do you have commit rights? Carl Eugen
On Sat, Jan 8, 2011 at 3:14 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Nathan Caldwell <saintdev <at> gmail.com> writes:
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever
Since several people stated here Lagarith is broken by definition, what about a warning in decode_init()?
So are a lot of other codecs we claim to support... Jason
On 8 Jan 2011, at 21:24, Jason Garrett-Glaser <jason at x264.com> wrote:
On Sat, Jan 8, 2011 at 3:14 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Nathan Caldwell <saintdev <at> gmail.com> writes:
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever
Since several people stated here Lagarith is broken by definition, what about a warning in decode_init()?
So are a lot of other codecs we claim to support...
However those at least aren't advertised as lossless codecs...
On Sun, Jan 09, 2011 at 10:26:51AM +0100, Reimar D?ffinger wrote:
On 8 Jan 2011, at 21:24, Jason Garrett-Glaser <jason at x264.com> wrote:
On Sat, Jan 8, 2011 at 3:14 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Nathan Caldwell <saintdev <at> gmail.com> writes:
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever
Since several people stated here Lagarith is broken by definition, what about a warning in decode_init()?
So are a lot of other codecs we claim to support...
However those at least aren't advertised as lossless codecs...
A codec using float in C is not lossless. you can use float in x86 asm and be lossless or C without float [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
On 1/8/11 12:24 PM, Jason Garrett-Glaser wrote:
On Sat, Jan 8, 2011 at 3:14 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Nathan Caldwell <saintdev <at> gmail.com> writes:
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or whatever
Since several people stated here Lagarith is broken by definition, what about a warning in decode_init()?
So are a lot of other codecs we claim to support...
Which ones ? -- Baptiste COUDURIER Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA FFmpeg maintainer http://www.ffmpeg.org
On Jan 8, 2011 1:14 PM, "Carl Eugen Hoyos" <cehoyos at ag.or.at> wrote:
Nathan Caldwell <saintdev <at> gmail.com> writes:
If people want we could mark it with CODEC_CAP_EXPERIMENTAL or
whatever
Since several people stated here Lagarith is broken by definition, what
about a
warning in decode_init()?
This sounds fine to me.
Do you have commit rights?
No, I don't. -- -Nathan Caldwell
Michael Niedermayer <michaelni <at> gmx.at> writes:
I suggest we commit this to main svn and continue improvments from there that way its easier for other people to help and its harder to become forgotten and rot
I committed the decoder adding three consts and a release_buffer(), please don't shoot me if it was wrong. Carl Eugen
On Jan 8, 2011 2:24 PM, "Carl Eugen Hoyos" <cehoyos at ag.or.at> wrote:
Michael Niedermayer <michaelni <at> gmx.at> writes:
I suggest we commit this to main svn and continue improvments from there that way its easier for other people to help and its harder to become
forgotten
and rot
I committed the decoder adding three consts and a release_buffer(), please don't shoot me if it was wrong.
Thanks Carl! -- -Nathan Caldwell
On 01/09/2011 09:28 AM, Nathan Caldwell wrote:
On Jan 8, 2011 2:24 PM, "Carl Eugen Hoyos"<cehoyos at ag.or.at> wrote:
Michael Niedermayer<michaelni<at> gmx.at> writes:
I suggest we commit this to main svn and continue improvments from there that way its easier for other people to help and its harder to become
forgotten
and rot
I committed the decoder adding three consts and a release_buffer(), please don't shoot me if it was wrong.
Thanks Carl!
Nice. Could you add one (or more) fate tests for it? -Vitor
Nathan Caldwell <saintdev <at> gmail.com> writes:
I committed the decoder adding three consts and a release_buffer(), please don't shoot me if it was wrong.
Thanks Carl!
Could you comment on why our oldest sample doesn't work: http://samples.mplayerhq.hu/V-codecs/lagarith/lagarith.avi Carl Eugen
On Sun, Jan 9, 2011 at 1:45 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
Nathan Caldwell <saintdev <at> gmail.com> writes:
I committed the decoder adding three consts and a release_buffer(), please don't shoot me if it was wrong.
Thanks Carl!
Could you comment on why our oldest sample doesn't work: http://samples.mplayerhq.hu/V-codecs/lagarith/lagarith.
[lagarith @ 01c821f0] Unsupported Lagarith frame type: 0x4 Last message repeated 77 times Input #0, avi, from '..\..\lagarith.avi': Duration: 00:00:09.14, start: 0.000000, bitrate: 16701 kb/s Stream #0.0: Video: lagarith, 480x256, 29.97 fps, 29.97 tbr, 29.97 tbn, 29.97 tbc That sample is RGB24. The decoder only supports YV12. -- -Nathan Caldwell
On Thu, Oct 15, 2009 at 08:42:54PM -0600, Nathan Caldwell wrote:
On Tue, Oct 13, 2009 at 3:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Sep 22, 2009 at 11:54:03PM -0600, Nathan Caldwell wrote:
Here are the latest Lagarith patches. The only changes to the rangecoder from the previous patch are cosmetic (K&R formatting). The decoder includes Loren's floating point emulation. Thanks to everyone, and especially Loren and Reimar, for all their help.
+ int prob[258]; /*!< Table of cumulative probability for each symbol. */
this is filled with uint32_t giving nice negative probs
Fixed.
+/* compute the 52bit mantissa of 1/(double)denom */ +static uint64_t softfloat_reciprocal(uint32_t denom)
comment not doxygen compatible and this really needs some explanation of why it is used instead of 1.0/denom in the doxy
Updated. It would be nice if Loren could comment on these descriptions.
+static uint32_t lag_decode_prob(GetBitContext *gb) +{ + static const uint8_t series[] = { 1, 2, 3, 5, 8, 13, 21, 34 };
where is series[7] accessed?
It isn't. The table was copied from reference. Removed.
+ if (bits <= 0) + return 0; + if (bits > 31) { + /* This is most likely an error, just use the first 32 bits */ + bits = 31; + }
are these not error conditions that warrent a error message and failure?
In the case of bits==0, no. Any other case (bits < 0 or bits >31), yes. This should be fixed now.
+ + value = get_bits_long(gb, bits); + value |= 1 << bits; + + return value - 1; +} + +static int lag_read_prob_header(lag_rac *rac, GetBitContext *gb) +{ + int i, j; + int prob = 0; + int scale_factor = 0; + unsigned cumul_prob = 0; + unsigned cumulative_target = 1; + unsigned scaled_cumul_prob = 0; + + rac->prob[0] = 0; + rac->prob[257] = UINT_MAX; + /* Read probabilities from bitstream */ + for (i = 1; i < 257; i++) { + rac->prob[i] = lag_decode_prob(gb);
+ cumul_prob += rac->prob[i];
can overflow
In this case the reference decoder would also overflow. Would you prefer to match the reference, or not overflow?
I dont mind either way as long as its no exploitable, cant crash and doesnt end in an infinite loop.
+ if (!rac->prob[i]) {
+ prob = lag_decode_prob(gb); + if (i + prob >= 258) + prob = 257 - i;
integer overflow, segfault below
Fixed.
and isnt this check supposed to be an error condition instead of silent cliping?
In a perfect world, yes. Again, just matching the behavior of the reference decoder.
[...]
also getting rid of *step and doing +=step might be faster and then variables like zeros could be tried to be moved onto the stack
This is what I originally had, however it introduces too many divisions in the calculation of the zero runs.
i is used without *step only for a comparission against width, width surely can be multiplied by step to avoid a division there. Am i missing something? [...]
+// decodes a byte +static inline int lag_get_rac(lag_rac *l)
// is not picked up by doxygen yet i think ...
+{ + unsigned range_scaled, low_scaled, div; + int val; + uint8_t shift; + + lag_rac_refill(l); + + range_scaled = l->range >> l->scale; + + if (l->low >= range_scaled * l->prob[255]) { + val = 255; + l->range -= range_scaled * l->prob[255]; + } else { + /* val = 0 is frequent enough to deserve a shortcut */ + if (l->low < range_scaled * l->prob[1]) { + val = 0;
isnt 0 more common than 255? and should thus be checked before on a shorter code path?
+ } else { + /* FIXME __builtin_clz is ~20% faster here, but not allowed in generic code. */
+ shift = 30 - av_log2(range_scaled);
table[ l->range>>24 ] could be used and would need just 128 entries or so
+ div = ((range_scaled << shift) + (1 << 23) - 1) >> 23; + /* low>>24 ensures that any cases too big for exact FASTDIV are + * under- rather than over-estimated + */ + low_scaled = FASTDIV(l->low - (l->low >> 24), div); + shift -= 23 + l->hash_shift;
23 + l->hash_shift can be factored out of the function as hash_shift is a "constant"
+ shift &= 31; + low_scaled = (low_scaled << shift) | (low_scaled >> (32 - shift)); + /* low_scaled is now a lower bound of low/range_scaled */ + val = l->range_hash[(uint8_t) low_scaled];
+ while (l->low >= range_scaled * l->prob[val + 1]) + val++;
is there some (small) bound on the number of iterations of this? if so it could be replaced by a if() or 2 ... [...]
+/** + * Decode a frame. + * @param avctx codec context + * @param data output AVFrame + * @param data_size size of output data or 0 if no picture is returned + * @param avpkt input packet + * @return number of consumed bytes on success or negative if decode fails + */ +static int lag_decode_frame(AVCodecContext *avctx, + void *data, int *data_size, AVPacket *avpkt) +{ + const uint8_t *buf = avpkt->data; + int buf_size = avpkt->size; + LagarithContext *l = avctx->priv_data; + AVFrame *const p = &l->picture; + uint8_t frametype = 0; + uint32_t offset_gu = 0, offset_bv = 0, offset_ry = 9; + + AVFrame *picture = data; +
+ if (!l->avctx) + l->avctx = avctx;
that can be done in lag_decode_init() [...] -- 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 then the original author, trying to rewrite it will not make it better.
On Sat, 24 Oct 2009, Michael Niedermayer wrote:
On Thu, Oct 15, 2009 at 08:42:54PM -0600, Nathan Caldwell wrote:
+ if (l->low >= range_scaled * l->prob[255]) { + val = 255; + l->range -= range_scaled * l->prob[255]; + } else { + /* val = 0 is frequent enough to deserve a shortcut */ + if (l->low < range_scaled * l->prob[1]) { + val = 0;
isnt 0 more common than 255? and should thus be checked before on a shorter code path?
Yes.
+ shift &= 31; + low_scaled = (low_scaled << shift) | (low_scaled >> (32 - shift)); + /* low_scaled is now a lower bound of low/range_scaled */ + val = l->range_hash[(uint8_t) low_scaled];
+ while (l->low >= range_scaled * l->prob[val + 1]) + val++;
is there some (small) bound on the number of iterations of this? if so it could be replaced by a if() or 2 ...
No. Amortized cost is bounded at 1 iteration, but the max for any given call can be up to 254. i.e. range_hash can be very wrong, but only for low-probability symbols. --Loren Merritt
participants (9)
-
baptiste.coudurier＠gmail.com -
cehoyos＠ag.or.at -
diego＠biurrun.de -
jason＠x264.com -
lorenm＠u.washington.edu -
michaelni＠gmx.at -
Reimar.Doeffinger＠gmx.de -
saintdev＠gmail.com -
vitor1001＠gmail.com