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

Matthew Einhorn moiein2000 at gmail.com
Sat Aug 20 23:06:45 CEST 2011


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?

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?

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

Thanks,
Matt

>
> keeps patches small and the final code is free of code duplication
> thanks
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No great genius has ever existed without some touch of madness. -- Aristotle
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk5O4goACgkQYR7HhwQLD6sRKQCggXi+1Nw3w8kckyoIkycbU357
> Fp0AnRSGZA2DQMhQj7Y8Ua+nYia/L+yD
> =lQpp
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>


More information about the ffmpeg-devel mailing list