[FFmpeg-devel] [PATCH] Fixes avcodec_find_best_pix_fmt() with more than 64 pix fmts defined

Matthew Einhorn moiein2000 at gmail.com
Sun Aug 21 20:45:54 CEST 2011


On Sun, Aug 21, 2011 at 2:19 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Aug 21, 2011 at 01:22:09PM -0400, Matthew Einhorn wrote:
>> On Sun, Aug 21, 2011 at 12:09 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Sat, Aug 20, 2011 at 05:06:45PM -0400, Matthew Einhorn wrote:
>> >> On Fri, Aug 19, 2011 at 6:22 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >> > On Wed, Aug 17, 2011 at 08:50:50PM -0400, Matthew Einhorn wrote:
>> >> >> 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
>> >> >> >
>> >> >> >
>> >> >
>> >> >>  avcodec.h    |   34 ++++++++++++++++++++++++++++++++++
>> >> >>  imgconvert.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>> >> >>  2 files changed, 80 insertions(+)
>> >> >> a2185b7288f78de01eaf39b89f5de1d313643607  0001-Adds-a-new-pix-fmt-function-that-selects-the-best-de.patch
>> >> >> From a873e791be7802b110ac44e279b904d28e2e2fb4 Mon Sep 17 00:00:00 2001
>> >> >> From: Matthew Einhorn <moiein2000 at gmail.com>
>> >> >> Date: Wed, 17 Aug 2011 20:14:54 -0400
>> >> >> Subject: [PATCH] Adds a new pix fmt function that selects the best dest fmt
>> >> >>  from among 2 formats to convert to, given a certain src
>> >> >>  format.
>> >> >>
>> >> >> Similar to avcodec_find_best_pix_fmt(), but instead only compares two destination
>> >> >> pix fmts and selects one of these fmts as the best dest format when converting from
>> >> >> a given source format. Also, as opposed to avcodec_find_best_pix_fmt() which supports
>> >> >> only the first 64 defined pixel formats, this supports as input any of the defined
>> >> >> pixel formats.
>> >> >>
>> >> >> Signed-off-by: Matthew Einhorn <moiein2000 at gmail.com>
>> >> >> ---
>> >> >>  libavcodec/avcodec.h    |   34 ++++++++++++++++++++++++++++++++++
>> >> >>  libavcodec/imgconvert.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>> >> >>  2 files changed, 80 insertions(+), 0 deletions(-)
>> >> >
>> >> > The old function should be replaced by the new and the old then
>> >> > implemented by using the new
>> >>
>> >> Should I implement new and re-implement old in one patch or in two patches?
>> >
>> > both in one patch
>> >
>> >
>> >>
>> >> Also, the old function makes use of an internal
>> >> avcodec_find_best_pix_fmt1() function which is used "only" by the old
>> >> function, can I remove that?
>> >
>> > yes but that should be a seperate patch
>> >
>> >
>> >>
>> >> Finally, the new function allows the input of PIX_FMT_NONE as a
>> >> destination format because it would simplify the use of it (if "one"
>> >> of the dest pix fmts can be -1) because you'd have to do less error
>> >> checking. However, to do that avcodec_get_pix_fmt_loss() also needs to
>> >> support the "dst_pix_fmt" parameter to be PIX_FMT_NONE (otherwise I
>> >> need to check for it in the new function). Would it be appropriate to
>> >> patch avcodec_get_pix_fmt_loss() to include a line of code at the
>> >> start where if the dest format is -1 it returns the maximum loss or
>> >> should it stay as undefined behavior (since arrays then get indexed by
>> >> -1)?
>> >
>> > yes, of course
>> >
>>
>> All three patches attached
>> Thanks,
>> Matt
>
>>  avcodec.h    |   34 ++++++++++++++++++++++++++++++++++
>>  imgconvert.c |   47 ++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 74 insertions(+), 7 deletions(-)
>> a340d8792706166253efe4bbbcf411a00bf5a0f2  0001-Adds-a-new-pix-fmt-function-that-selects-the-best-de.patch
>> From 830379b3867e24bbe62fe2e305a3efb143ba2086 Mon Sep 17 00:00:00 2001
>> From: Matthew Einhorn <moiein2000 at gmail.com>
>> Date: Sun, 21 Aug 2011 12:42:11 -0400
>> Subject: [PATCH 1/3] Adds a new pix fmt function that selects the best dest
>>  fmt from among 2 formats to convert to, given a certain
>>  src format.
>>
>> Similar to avcodec_find_best_pix_fmt(), but instead only compares two destination
>> pix fmts and selects one of these fmts as the best dest format when converting from
>> a given source format. Also, as opposed to avcodec_find_best_pix_fmt() which supports
>> only the first 64 defined pixel formats, this supports as input any of the defined
>> pixel formats.
>>
>> Also replaces the old function to rely on the new function.
>>
>> Signed-off-by: Matthew Einhorn <moiein2000 at gmail.com>
>> ---
>>  libavcodec/avcodec.h    |   34 ++++++++++++++++++++++++++++++++++
>>  libavcodec/imgconvert.c |   47 ++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 74 insertions(+), 7 deletions(-)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 4f0ed2d..9d9c3c5 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -3508,8 +3508,42 @@ int avcodec_get_pix_fmt_loss(enum PixelFormat dst_pix_fmt, enum PixelFormat src_
>>   * @param[out] loss_ptr Combination of flags informing you what kind of losses will occur.
>>   * @return The best pixel format to convert to or -1 if none was found.
>>   */
>> +
>>  enum PixelFormat avcodec_find_best_pix_fmt(int64_t pix_fmt_mask, enum PixelFormat src_pix_fmt,
>>                                int has_alpha, int *loss_ptr);
>> +/**
>> + * Find the best pixel format to convert to given a certain source pixel
>> + * format and a selection of two destination pixel formats. When converting from
>> + * one pixel format to another, information loss may occur.  For example, when converting
>> + * from RGB24 to GRAY, the color information will be lost. Similarly, other losses occur when
>> + * converting from some formats to other formats. avcodec_find_best_pix_fmt2() selects which of
>> + * the given pixel formats should be used to suffer the least amount of loss.
>> + *
>> + * If one of the destination formats is PIX_FMT_NONE the other pixel format (if valid) will be
>> + * returned. If two destination pixel formats have similar losses, the one with smallest
>> + * average bit depth will be chosen.
>> + *
>> + * @code
>> + * src_pix_fmt = PIX_FMT_YUV420P;
>> + * dst_pix_fmt1= PIX_FMT_RGB24;
>> + * dst_pix_fmt2= PIX_FMT_GRAY8;
>> + * dst_pix_fmt3= PIX_FMT_RGB8;
>> + * loss= FF_LOSS_CHROMA; // don't care about chroma loss, so chroma loss will be ignored.
>> + * dst_pix_fmt = avcodec_find_best_pix_fmt2(dst_pix_fmt1, dst_pix_fmt2, src_pix_fmt, alpha, &loss);
>> + * dst_pix_fmt = avcodec_find_best_pix_fmt2(dst_pix_fmt, dst_pix_fmt3, src_pix_fmt, alpha, &loss);
>> + * @endcode
>> + *
>> + * @param[in] dst_pix_fmt1 One of the two destination pixel formats to choose from
>> + * @param[in] dst_pix_fmt2 The other of the two destination pixel formats to choose from
>> + * @param[in] src_pix_fmt Source pixel format
>> + * @param[in] has_alpha Whether the source pixel format alpha channel is used.
>> + * @param[in, out] loss_ptr Combination of loss flags. In: selects which of the losses to ignore, i.e.
>> + *                               NULL or value of zero means we care about all losses. Out: the loss
>> + *                               that occurs when converting from src to selected dst pixel format.
>> + * @return The best pixel format to convert to or -1 if none was found.
>> + */
>> +enum PixelFormat avcodec_find_best_pix_fmt2(enum PixelFormat dst_pix_fmt1, enum PixelFormat dst_pix_fmt2,
>> +                                            enum PixelFormat src_pix_fmt, int has_alpha, int *loss_ptr)
>>
>>  #define FF_ALPHA_TRANSP       0x0001 /* image has some totally transparent pixels */
>>  #define FF_ALPHA_SEMI_TRANSP  0x0002 /* image has some transparent pixels */
>> diff --git a/libavcodec/imgconvert.c b/libavcodec/imgconvert.c
>> index 9efed50..25cb2cb 100644
>> --- a/libavcodec/imgconvert.c
>> +++ b/libavcodec/imgconvert.c
>> @@ -455,10 +455,30 @@ static enum PixelFormat avcodec_find_best_pix_fmt1(int64_t pix_fmt_mask,
>>  }
>>
>>  enum PixelFormat avcodec_find_best_pix_fmt(int64_t pix_fmt_mask, enum PixelFormat src_pix_fmt,
>> -                              int has_alpha, int *loss_ptr)
>> +                                            int has_alpha, int *loss_ptr)
>>  {
>>      enum PixelFormat dst_pix_fmt;
>> -    int loss_mask, i;
>> +    int i;
>> +
>
>> +    if (loss_ptr)    /* all losses count (for backward compatibility) */
>
> tabs are forbidden in ffmpeg git
>
>
>> +        *loss_ptr= 0;
>
>> +    i= -1;
>> +    dst_pix_fmt= PIX_FMT_NONE; /* so first iteration doesn't have to be treated special */
>> +    while (1){
>> +        while(++i < FFMIN(PIX_FMT_NB, 64) && !(pix_fmt_mask & (1ULL << i)));
>> +        if (i < FFMIN(PIX_FMT_NB, 64))
>> +            dst_pix_fmt= avcodec_find_best_pix_fmt2(dst_pix_fmt, i, src_pix_fmt, has_alpha, loss_ptr);
>> +        else
>> +            break;
>> +    }
>
> The following is simpler:
>
> for(i=0; i< FFMIN(PIX_FMT_NB, 64); i++){
>    if(pix_fmt_mask & (1ULL << i))
>        ...
> }
>
>
>
>> +    return dst_pix_fmt;
>> +}
>> +
>> +enum PixelFormat avcodec_find_best_pix_fmt2(enum PixelFormat dst_pix_fmt1, enum PixelFormat dst_pix_fmt2,
>> +                                            enum PixelFormat src_pix_fmt, int has_alpha, int *loss_ptr)
>> +{
>> +    enum PixelFormat dst_pix_fmt;
>> +    int loss1, loss2, i, loss_mask;
>>      static const int loss_mask_order[] = {
>>          ~0, /* no loss first */
>>          ~FF_LOSS_ALPHA,
>> @@ -469,18 +489,31 @@ enum PixelFormat avcodec_find_best_pix_fmt(int64_t pix_fmt_mask, enum PixelForma
>>          0,
>>      };
>>
>> +    loss_mask= loss_ptr?~*loss_ptr:~0; /* use loss mask if provided */
>> +    dst_pix_fmt = PIX_FMT_NONE;
>>      /* try with successive loss */
>>      i = 0;
>>      for(;;) {
>> -        loss_mask = loss_mask_order[i++];
>> -        dst_pix_fmt = avcodec_find_best_pix_fmt1(pix_fmt_mask, src_pix_fmt,
>> -                                                 has_alpha, loss_mask);
>> +        loss1 = avcodec_get_pix_fmt_loss(dst_pix_fmt1, src_pix_fmt, has_alpha) & loss_mask_order[i] & loss_mask;
>> +        loss2 = avcodec_get_pix_fmt_loss(dst_pix_fmt2, src_pix_fmt, has_alpha) & loss_mask_order[i] & loss_mask;
>
> theres no need to call these 2 in a loop, their value doesnt change
>
>
>> +
>> +        if (loss1 == 0 && loss2 == 0){       /* use format with smallest depth */
>> +            dst_pix_fmt= avg_bits_per_pixel(dst_pix_fmt2)<avg_bits_per_pixel(dst_pix_fmt1)?dst_pix_fmt2:dst_pix_fmt1;
>> +        } else if (loss1 == 0 || loss2 == 0) {       /* use format with no loss */
>> +            dst_pix_fmt= loss2?dst_pix_fmt1:dst_pix_fmt2;
>> +        }
>> +
>>          if (dst_pix_fmt >= 0)
>>              goto found;
>> -        if (loss_mask == 0)
>> +        if (loss_mask_order[i++] == 0)
>>              break;
>>      }
>> -    return PIX_FMT_NONE;
>
>> +
>> +    /* none found only if one or both of dst_pix_fmts are -1, so return the other (valid) dst_pix_fmt that isn't -1 */
>> +    if (dst_pix_fmt1 <PIX_FMT_NB && dst_pix_fmt1 > PIX_FMT_NONE)
>> +        dst_pix_fmt= dst_pix_fmt1;
>> +    if (dst_pix_fmt2 <PIX_FMT_NB && dst_pix_fmt2 > PIX_FMT_NONE)
>> +        dst_pix_fmt= dst_pix_fmt2;
>
> this should not be necessary, invalid formats should already have the
> worst score, forcing the other to be selected
>

Will fix the others, but for this last one, what if you have a pixel
format that has such bad losses that it would only get selected when
we AND it with the loss mask of 0 (last element in loss array). In
that case it would be equivalent to an invalid format in terms of
loss. Should I not worry about such a pixel format?

Thanks,
Matt


More information about the ffmpeg-devel mailing list