[Libav-user] PIX_FMT_NB>64 so avcodec_find_best_pix_fmt(int64_t pix_fmt_mask, ...) wraps back to zero after bit 64.

[Matthew Einhorn]

Hi,

This is basically a bug which appeared between git 9c2651a (July 23)
and git faa3381 (July 28). It still worked in the 9c2651a  git.

The problem is that in avcodec_find_best_pix_fmt1() the loop looks at
all bits from 0 - PIX_FMT_NB but since PIX_FMT_NB is larger than 64,
after the 64th iteration we overflow back to the first bit and the
first pixel format but the loop thinks that we're at say pix fmt 68.
The following code demonstrates the problem and workaround.

Code:
int nLoss;
PixelFormat pix= avcodec_find_best_pix_fmt(1ULL << PIX_FMT_GRAY8 ,
PIX_FMT_YUV420P, 0, &nLoss);
std::cout<<pix<<std::endl;
pix= pix>64?(PixelFormat)(pix-64):pix;
std::cout<<pix<<std::endl;

Output:
72
8

I noticed that the code in imgconvert.c got updated between July 23-28
and I looked through to see if I can find why it stopped working, but
couldn't. In fact, I'm not sure why it ever worked once PIX_FMT_NB was
larger than 64.



An additional question, since PIX_FMT_NB is larger than 64 it means we
cannot use this function on any new pixel formats since they don't fit
within the 64 bit mask, unless I'm missing something. Would it be
possible to add/replace these functions with something like the
following? It basically replaces the mask with an array. I know an
array wastes bit space, but you won't have to search through all 64
flags anymore since you know how many elements are in the array so it
should be a bit faster. I have never used git, otherwise I'd try to
add it as a patch.


static enum PixelFormat avcodec_find_best_pix_fmt_alt1(enum
PixelFormat *dst_pix_fmts,
                                      enum PixelFormat src_pix_fmt,
                                      int has_alpha,
                                      int loss_mask)
{
    int dist, i, loss, min_dist;
    enum PixelFormat dst_pix_fmt;

	/* find exact color match with smallest size */
	dst_pix_fmt = PIX_FMT_NONE;
	min_dist = 0x7fffffff;
	for(i = 0;dst_pix_fmts[i] != PIX_FMT_NONE; i++) {
		loss = avcodec_get_pix_fmt_loss(dst_pix_fmts[i], src_pix_fmt,
has_alpha) & loss_mask;
		if (loss == 0) {
			dist = avg_bits_per_pixel(dst_pix_fmts[i]);
			if (dist < min_dist) {
				min_dist = dist;
				dst_pix_fmt = dst_pix_fmts[i];
			}
		}
	}
    return dst_pix_fmt;
}

enum PixelFormat avcodec_find_best_pix_fmt_alt(enum PixelFormat
*dst_pix_fmts, enum PixelFormat src_pix_fmt,
                              int has_alpha, int *loss_ptr)
{
    enum PixelFormat dst_pix_fmt;
    int loss_mask, i;
    static const int loss_mask_order[] = {
        ~0, /* no loss first */
        ~FF_LOSS_ALPHA,
        ~FF_LOSS_RESOLUTION,
        ~(FF_LOSS_COLORSPACE | FF_LOSS_RESOLUTION),
        ~FF_LOSS_COLORQUANT,
        ~FF_LOSS_DEPTH,
        0,
    };

	if (!dst_pix_fmts)
		return PIX_FMT_NONE;

    /* try with successive loss */
    i = 0;
    for(;;) {
        loss_mask = loss_mask_order[i++];
        dst_pix_fmt = avcodec_find_best_pix_fmt_alt1(dst_pix_fmts, src_pix_fmt,
                                                 has_alpha, loss_mask);
        if (dst_pix_fmt >= 0)
            goto found;
        if (loss_mask == 0)
            break;
    }
    return PIX_FMT_NONE;
 found:
    if (loss_ptr)
        *loss_ptr = avcodec_get_pix_fmt_loss(dst_pix_fmt, src_pix_fmt,
has_alpha);
    return dst_pix_fmt;
}

Thanks,
Matt