[FFmpeg-devel] [PATCH] Fixes avcodec_find_best_pix_fmt() with more than 64 pix fmts defined
Michael Niedermayer
michaelni at gmx.at
Sat Aug 20 00:22:02 CEST 2011
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
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110820/021b1975/attachment.asc>
More information about the ffmpeg-devel
mailing list