[FFmpeg-devel] [PATCH] Fixes avcodec_find_best_pix_fmt() with more than 64 pix fmts defined
Matthew Einhorn
moiein2000 at gmail.com
Thu Aug 18 02:50:50 CEST 2011
On Wed, Aug 17, 2011 at 9:32 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Wed, Aug 17, 2011 at 10:43:34AM +0200, Stefano Sabatini wrote:
>> On date Wednesday 2011-08-17 02:38:37 -0400, Matthew Einhorn encoded:
>> > Hi,
>> >
>> > As stated, this patch (attached) should fix
>> > avcodec_find_best_pix_fmt() so that now with more than 64 pix fmts,
>> > the function should be able to work for the first 64 fmts. Currently
>> > the function returns a bad formats, for example,
>> > avcodec_find_best_pix_fmt(1ULL << PIX_FMT_GRAY8, PIX_FMT_YUV420P, 0,
>> > &nLoss) returns 72 instead of 8.
>> >
>> > Please be gentle as this is the first time I used git or submitted a
>> > patch. If this works fine I'll try to submit a patch for a second
>> > avcodec_find_best_pix_fmt_alt function that'll support more than 64
>> > formats as input to the function.
>>
>> You're welcome. And yes I agree we should replace the function with a
>> sane variant, for example:
>>
>> enum PixelFormat avcodec_find_best_pix_fmt2(int *pix_fmts, int pix_fmts_nb,
>> enum PixelFormat src_pix_fmt, ...);
>>
>> you pass a list of pixel formats (specifying the size, or
>> alternatively setting the last element to -1), no hardcoded limit on
>> the number of pixel formats.
>
> depending on how its used it might be simpler to have a function
> comparing 2 pixel formats
> int is_better_pix_fmt(enum PixelFormat *best, enum PixelFormat try,
> enum PixelFormat src_pix_fmt, ...);
>
> That would avoid having to build the array if it has to be build
>
That makes sense, however, the function (patch attached) should have
more than one destination
pixel format to choose from. The reason is that we already have a
avcodec_get_pix_fmt_loss() function
that could be used to find which pixel format is better as compared to another.
avcodec_find_best_pix_fmt() is typically used when you have a source
and multiple destination formats and you
like to find the best destination format, in that case comparing the
source to a single dest pixel fmt won't be helpful
directly because you want to compare multiple destination formats in
the "context" of the source format. I think the function
added in the patch solves that.
See in the documentation of the function (in the patch) how this would
allow to find the best pixel format from a list of formats.
In addition, the function allows the user to select which type of loss
is acceptable to them (loss_ptr) and they aren't locked in to the
losses as enumerated
in the loss_mask_order array.
Also, I didn't change the loss_mask_order array that enumerates losses
in increasing order. However, should that be edited? I'm not sure
why that order was selected, but I guess now is a good time as any to
look at that and possibly improve/increase that array? I don't know
much
about pix fmts so I don't know if that's already prefect.
Thanks for the application of the previous patch,
Matt
>
>>
>> >
>> > Thanks,
>> > Matt
>>
>> > From 076c8f936a46ea42cd4242eb4df2db265653d972 Mon Sep 17 00:00:00 2001
>> > From: Matthew Einhorn <moiein2000 at gmail.com>
>> > Date: Wed, 17 Aug 2011 01:58:33 -0400
>> > Subject: [PATCH] Fixes avcodec_find_best_pix_fmt() when there's more than 64
>> > pixel formats.
>> >
>> > This fixed the problem where if there's more than 64 pixel formats defined
>> > avcodec_find_best_pix_fmt() returns the wrong pixel format.
>> >
>> > Signed-off-by: Matthew Einhorn <moiein2000 at gmail.com>
>> > ---
>> > libavcodec/avcodec.h | 4 +++-
>> > libavcodec/imgconvert.c | 2 +-
>> > 2 files changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> > index 4f0ed2d..f7d1520 100644
>> > --- a/libavcodec/avcodec.h
>> > +++ b/libavcodec/avcodec.h
>> > @@ -3496,9 +3496,11 @@ int avcodec_get_pix_fmt_loss(enum PixelFormat dst_pix_fmt, enum PixelFormat src_
>> > * The pixel formats from which it chooses one, are determined by the
>> > * pix_fmt_mask parameter.
>> > *
>> > + * Note, only the first 64 pixel formats will fit in pix_fmt_mask
>> > + *
>> > * @code
>> > * src_pix_fmt = PIX_FMT_YUV420P;
>> > - * pix_fmt_mask = (1 << PIX_FMT_YUV422P) || (1 << PIX_FMT_RGB24);
>> > + * pix_fmt_mask = (1 << PIX_FMT_YUV422P) | (1 << PIX_FMT_RGB24);
>> > * dst_pix_fmt = avcodec_find_best_pix_fmt(pix_fmt_mask, src_pix_fmt, alpha, &loss);
>> > * @endcode
>> > *
>> > diff --git a/libavcodec/imgconvert.c b/libavcodec/imgconvert.c
>> > index 9efed50..a4197cb 100644
>> > --- a/libavcodec/imgconvert.c
>> > +++ b/libavcodec/imgconvert.c
>> > @@ -439,7 +439,7 @@ static enum PixelFormat avcodec_find_best_pix_fmt1(int64_t pix_fmt_mask,
>> > /* find exact color match with smallest size */
>> > dst_pix_fmt = PIX_FMT_NONE;
>> > min_dist = 0x7fffffff;
>> > - for(i = 0;i < PIX_FMT_NB; i++) {
>> > + for(i = 0;i < FFMIN(PIX_FMT_NB, 64); i++) {
>> > if (pix_fmt_mask & (1ULL << i)) {
>> > loss = avcodec_get_pix_fmt_loss(i, src_pix_fmt, has_alpha) & loss_mask;
>> > if (loss == 0) {
>>
>> Looks fine to me, I'm going to apply soon if I read no comments (maybe
>> Michael?).
>
> LGTM
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Observe your enemies, for they first find out your faults. -- Antisthenes
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk5LwucACgkQYR7HhwQLD6uSOwCfVwo3VbBwbFOUq76z0tCwkuEq
> QzIAn2kZ37gkkztp6KYVyBHXZ0Rx/m2w
> =/zSG
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adds-a-new-pix-fmt-function-that-selects-the-best-de.patch
Type: application/octet-stream
Size: 5905 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110817/b68d2868/attachment.obj>
More information about the ffmpeg-devel
mailing list