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

[Stefano Sabatini]

On date Saturday 2011-08-13 01:01:45 -0400, Matthew Einhorn encoded:
> 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.

I did the changes to imgconvert.c, and I was aware of the issue, and
from my knowledge that code couldn't ever work with more than 64 pixel
formats.
 
> 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.

git diff
edit edit edit...
git commit -a
git send-email HASH~1..HASH

> 
> 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)

avcodec_find_best_pix_fmt() is a public function, so you can't change
it without breaking API/ABI compatibility. You need to add a separate
function - e.g. avcodec_find_best_pix_fmt2().

You're welcome to post a patch to ffmpeg-devel, or file a bug report,
but keep in mind, we tend to be *dreadly* busy, so a patch+review is
possibly a faster way to see it fixed, and you may learn something in
the process.

[...]