[FFmpeg-devel] [PATCH] CCITT Group 3 and 4 decompression
Michael Niedermayer
michaelni
Tue Dec 23 20:27:31 CET 2008
On Tue, Dec 23, 2008 at 09:00:57AM +0200, Kostya wrote:
> On Mon, Dec 22, 2008 at 09:06:37PM +0100, Michael Niedermayer wrote:
> > On Mon, Dec 22, 2008 at 08:15:06PM +0200, Kostya wrote:
> > > On Mon, Dec 22, 2008 at 01:13:53PM +0100, Michael Niedermayer wrote:
> > > > On Mon, Dec 22, 2008 at 08:31:55AM +0200, Kostya wrote:
> > > > > I need that for resolving issue 750 on roundup.
> [...]
> > > +{
> > > + static VLC_TYPE code_table1[528][2];
> > > + static VLC_TYPE code_table2[648][2];
> > > + int i;
> > > + if(ccitt_group3_2d_vlc.table)
> > > + return;
> >
> > this is not thread safe, this is not the last value written.
> >
> > and yes these functions can be called from several threads from
> > av_find_stream_info() ...
> > alternative solutions are welcome as well but as long as av_find_stream_info
> > does what it does all init must be thread safe.
>
> now it should be thread safe
>
> [...]
> >
> > > +static inline int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
> > > + int pix_left, int *runs)
> > > +{
> > > + int mode = 0, run = 0, t;
> > > + while(pix_left){
> > > + t = get_vlc2(gb, ccitt_vlc[mode].table, 9, 2);
> > > + run += t;
> > > + if(t == -1){
> > > + av_log(avctx, AV_LOG_ERROR, "Incorrect code @ %d\n",avctx->width -pix_left);
> > > + return -1;
> > > + }
> > > + if(t >= 64)
> > > + continue;
> > > + pix_left -= run;
> > > + if(pix_left < 0){
> > > + av_log(avctx, AV_LOG_ERROR, "Run went out of bounds\n");
> > > + return -1;
> > > + }
> > > + *runs++ = run;
> > > + run = 0;
> > > + mode = !mode;
> > > + }
> > > + *runs++ = 0;
> > > + return 0;
> > > +}
> >
> > This still has 4 ifs on the most commonly excuted path, the code i provided
> > has 2, is there any reason why you do not use something based on just 2?
>
> Your code was wrong since it could not check for incorrect code (t = -1 was
> falling into t<64 condition).
>
> > And just the clarify, the "continue" path can only be executed once every
> > 64 pixels and thus is of less relevance speedwise.
>
> ?
> no, it's not a counter. For example, white run of 2000 pixels will be coded
> as two values: <1984><16>. See ITU T.4
>
> > also the code can end in a "infinite" and then segfaulting loop through the
> > continue
>
> Probably not. When bitreader is out of data it will return zeroes and there's no
> all-zero code in the list, so get_vlc2() will return -1 eventually.
>
> > > +
> > > +static inline int decode_group3_2d_line(AVCodecContext *avctx, GetBitContext *gb,
> > > + int pix_left, int *runs, const int *ref)
> > > +{
> >
> > the function does not look speed critical enough for the inline ...
> >
> >
> > [...]
> >
> > > +
> > > +static void put_line(uint8_t *dst, int size, int width, const int *runs)
> > > +{
> > > + PutBitContext pb;
> > > + int run, t, mode = 0, pix_left = width;
> > > +
> > > + init_put_bits(&pb, dst, size*8);
> > > + while(pix_left > 0){
> > > + run = *runs++;
> > > + pix_left -= run;
> >
> > > + while(run){
> > > + t = FFMIN(run, 16);
> > > + put_sbits(&pb, t, -mode);
> > > + run -= t;
> > > + }
> >
> > for(; run>=16; run-=16)
> > put_sbits(&pb, 16, -mode);
> > put_sbits(&pb, run, -mode);
> >
> >
> > [...]
> > > +int ff_ccitt_unpack_group3_1d(AVCodecContext *avctx,
> > > + const uint8_t *src, int srcsize,
> > > + uint8_t *dst, int height, int stride, int padded)
> > > +{
> > > + int j;
> > > + GetBitContext gb;
> > > + int *runs;
> > > +
> > > + runs = av_malloc(avctx->width * sizeof(runs[0]));
> > > + init_get_bits(&gb, src, srcsize*8);
> > > + for(j = 0; j < height; j++){
> > > + if(padded){
> > > + int bits = get_bits_count(&gb) & 7;
> > > + if(bits != 4)
> > > + skip_bits(&gb, (12 - bits) & 7);
> > > + }
> > > + if(get_bits(&gb, 12) != 1){
> > > + av_log(avctx, AV_LOG_ERROR, "EOL syncmarker not found, resyncing\n");
> > > + do{
> > > + skip_bits1(&gb);
> > > + }while(get_bits_count(&gb) < srcsize*8 && show_bits(&gb, 12) != 1);
> > > + if(get_bits_count(&gb) >= srcsize*8)
> > > + return -1;
> > > + }
> >
> > how is this supposed to work?
> > once a line is damaged and lost there can be 1 line less thus this can
> > still run out of data and fail.
>
> it's the question of what to do with damaged data
what do you prefer, no image or a image with a missing line?
or did i misunderstand you?
[...]
> @@ -103,6 +105,29 @@
> return -1;
> }
> }
> + if(s->compr == TIFF_CCITT_RLE || s->compr == TIFF_G3 || s->compr == TIFF_G4){
> + int i, ret = 0;
> + uint8_t *src2;
> +
> + src2 = av_malloc(size);
can be merged
[...]
> +static int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
> + int pix_left, int *runs)
> +{
> + int mode = 0, run = 0, t;
> + while(pix_left){
> + t = get_vlc2(gb, ccitt_vlc[mode].table, 9, 2);
> + run += t;
> + if(t == -1){
> + av_log(avctx, AV_LOG_ERROR, "Incorrect code\n");
> + return -1;
> + }
> + if(t >= 64)
> + continue;
> + pix_left -= run;
> + if(pix_left < 0){
> + av_log(avctx, AV_LOG_ERROR, "Run went out of bounds\n");
> + return -1;
> + }
> + *runs++ = run;
> + run = 0;
> + mode = !mode;
> + }
> + *runs++ = 0;
> + return 0;
> +}
still way more checks than neeeded
[...]
> +static void put_line(uint8_t *dst, int size, int width, const int *runs)
> +{
> + PutBitContext pb;
> + int run, mode = 0, pix_left = width;
> +
> + init_put_bits(&pb, dst, size*8);
> + while(pix_left > 0){
> + run = *runs++;
> + pix_left -= run;
> + for(; run >= 16; run -= 16)
> + put_sbits(&pb, 16, -mode);
> + if(run)
> + put_sbits(&pb, run, -mode);
for(; run > 16; run -= 16)
put_sbits(&pb, 16, -mode);
put_sbits(&pb, run, -mode);
> + mode = !mode;
> + }
> +}
> +
> +static int find_group3_syncmarker(GetBitContext *gb, int srcsize)
> +{
> + int ret = 0;
> + while(show_bits(gb, 12) != 1){
> + skip_bits1(gb);
> + ret = 1;
> + if(get_bits_count(gb) >= srcsize)
> + return -1;
> + }
> + skip_bits(gb, 12);
> + return ret;
> +}
int rem= srcsize - get_bits_count(gb);
uint32_t state=-1;
while(rem-- > 0){
state+= state + get_bits1();
if((state&0xFFF) == 1)
return 1;
}
return 0;
[...]
> +int ff_ccitt_unpack_group3_2d(AVCodecContext *avctx,
> + const uint8_t *src, int srcsize,
> + uint8_t *dst, int height, int stride)
> +{
> + int j;
> + GetBitContext gb;
> + int *runs, *ref;
> + int ret;
> +
> + runs = av_malloc((avctx->width + 1) * sizeof(runs[0]));
> + ref = av_malloc((avctx->width + 1) * sizeof(ref[0]));
> + runs[0] = 0;
> + ref[0] = 0;
> + ref[1] = avctx->width;
> + init_get_bits(&gb, src, srcsize*8);
> + for(j = 0; j < height; j++){
> + if(find_group3_syncmarker(&gb, srcsize*8) < 0)
> + break;
> + if(get_bits1(&gb))
> + ret = decode_group3_1d_line(avctx, &gb, avctx->width, runs + 1);
> + else
> + ret = decode_group3_2d_line(avctx, &gb, avctx->width, runs + 1, ref);
> + if(ret < 0){
> + av_free(runs);
> + av_free(ref);
> + return -1;
> + }
> + put_line(dst, stride, avctx->width, runs + 1);
> + FFSWAP(int*, runs, ref);
> + dst += stride;
> + }
> + av_free(runs);
> + av_free(ref);
> + return 0;
> +}
> +
> +int ff_ccitt_unpack_group4(AVCodecContext *avctx,
> + const uint8_t *src, int srcsize,
> + uint8_t *dst, int height, int stride)
> +{
> + int j;
> + GetBitContext gb;
> + int *runs, *ref;
> + int ret;
> +
> + runs = av_malloc((avctx->width + 1) * sizeof(runs[0]));
> + ref = av_malloc((avctx->width + 1) * sizeof(ref[0]));
> + runs[0] = 0;
> + ref[0] = 0;
> + ref[1] = avctx->width;
> + init_get_bits(&gb, src, srcsize*8);
> + for(j = 0; j < height; j++){
> + ret = decode_group3_2d_line(avctx, &gb, avctx->width, runs + 1, ref);
> + if(ret < 0){
> + av_free(runs);
> + av_free(ref);
> + return -1;
> + }
> + put_line(dst, stride, avctx->width, runs + 1);
> + FFSWAP(int*, runs, ref);
> + dst += stride;
> + }
> + av_free(runs);
> + av_free(ref);
> + return 0;
> +}
this function looks duplicated
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081223/205462e1/attachment.pgp>
More information about the ffmpeg-devel
mailing list