[MPlayer-cvslog] r34492 - in trunk: fmt-conversion.c libmpdemux/mp_taglists.c

[Reimar Döffinger]

On 4 Jan 2012, at 13:31, Reinhard Tartler <siretart at tauware.de> wrote:

> On Di, Jan 03, 2012 at 19:29:56 (CET), Reimar Döffinger wrote:
> 
>> On 3 Jan 2012, at 16:51, siretart <subversion at mplayerhq.hu> wrote:
>>> Author: siretart
>>> Date: Tue Jan  3 16:51:26 2012
>>> New Revision: 34492
>>> 
>>> Log:
>>> Allow compilation with Libav
>>> 
>>> Some CPP Macros and codec ids are not (yet) available in libav, so use
>>> them only if they are actually defined. This doesn't work for code ids,
>>> as they are defined as enums. Therefore, #ifdefs tests for the presence
>>> of the respective codec.
>>> 
>>> This approach should also allow to compile mplayer against earlier
>>> versions of FFmpeg.
>>> 
>>> Modified:
>>>  trunk/fmt-conversion.c
>>>  trunk/libmpdemux/mp_taglists.c
>>> 
>>> Modified: trunk/fmt-conversion.c
>>> ==============================================================================
>>> --- trunk/fmt-conversion.c    Mon Jan  2 17:48:18 2012    (r34491)
>>> +++ trunk/fmt-conversion.c    Tue Jan  3 16:51:26 2012    (r34492)
>>> @@ -57,9 +57,11 @@ static const struct {
>>>    {IMGFMT_RGB8,    PIX_FMT_BGR8},
>>>    {IMGFMT_RGB4,    PIX_FMT_BGR4},
>>>    {IMGFMT_BGR8,    PIX_FMT_PAL8},
>>> +#ifdef PIX_FMT_0RGB32
>>>    {IMGFMT_BGR32,   PIX_FMT_0RGB32},
>>> +#endif
>>> #if LIBAVUTIL_VERSION_INT >= AV_VERSION_INT(51, 20, 1)
>>> -    {IMGFMT_GBR24P,  PIX_FMT_GBR24P},
>>> +    {IMGFMT_GBR24P,  PIX_FMT_GBRP},
>>> #endif
>>>    {IMGFMT_YUY2,    PIX_FMT_YUYV422},
>>>    {IMGFMT_UYVY,    PIX_FMT_UYVY422},
>>> 
>>> Modified: trunk/libmpdemux/mp_taglists.c
>>> ==============================================================================
>>> --- trunk/libmpdemux/mp_taglists.c    Mon Jan  2 17:48:18 2012    (r34491)
>>> +++ trunk/libmpdemux/mp_taglists.c    Tue Jan  3 16:51:26 2012    (r34492)
>>> @@ -43,7 +43,9 @@ static const struct AVCodecTag mp_wav_ta
>>>    { CODEC_ID_COOK,              MKTAG('c', 'o', 'o', 'k')},
>>>    { CODEC_ID_DSICINAUDIO,       MKTAG('D', 'C', 'I', 'A')},
>>>    { CODEC_ID_EAC3,              MKTAG('E', 'A', 'C', '3')},
>>> +#ifdef CONFIG_FFWAVESYNTH_DECODER
>>>    { CODEC_ID_FFWAVESYNTH,       MKTAG('F', 'F', 'W', 'S')},
>>> +#endif
>>>    { CODEC_ID_G723_1,            MKTAG('7', '2', '3', '1')},
>>>    { CODEC_ID_INTERPLAY_DPCM,    MKTAG('I', 'N', 'P', 'A')},
>>>    { CODEC_ID_MLP,               MKTAG('M', 'L', 'P', ' ')},
>>> @@ -85,7 +87,9 @@ static const struct AVCodecTag mp_codeci
>>>    { CODEC_ID_DVVIDEO,           MKTAG('d', 'v', 's', 'd')},
>>>    { CODEC_ID_EAC3,              MKTAG('E', 'A', 'C', '3')},
>>>    { CODEC_ID_ESCAPE124,         MKTAG('E', '1', '2', '4')},
>>> +#ifdef CONFIG_ESCAPE130_DECODER
>>>    { CODEC_ID_ESCAPE130,         MKTAG('E', '1', '3', '0')},
>>> +#endif
>>>    { CODEC_ID_FLV1,              MKTAG('F', 'L', 'V', '1')},
>>>    { CODEC_ID_G729,              MKTAG('G', '7', '2', '9')},
>>>    { CODEC_ID_H264,              MKTAG('H', '2', '6', '4')},
>> 
>> Please revert, as I said in my reply to the patch, checking CONFIG_* is nonsense.
> 
> It seems I have misread your email. In particular, you said you would
> accept a non-broken solution but I did not see that you considered this
> approach broken.

No worries, misunderstandings happen.

> I'm therefore going to change the ifdefs to check for
> the LIBAVUTIL_VERSION_MICRO, as you suggest. Thinking more about it, it
> also helps with supporting the FFmpeg 0.8 releases, if that was a
> conern.

Yes, that seems fine.
I thought about more thorough solutions like just checking for the enum in configure which probably would qualify as the proper solution, but it seems a bit overkill.

>> Also, the ifdef for PIX_FMT is questionable, pixfmts generally are
>> enums, too, though in this case it works fine since it is one of the
>> formats that differ between little/big-endian, so it should at least
>> have a comment so nobody gets the idea to do the same thing for other
>> formats in the future where it will not work.
> 
> In order to avoid further bikeshedding, how about this?
> 
> Index: fmt-conversion.c
> ===================================================================
> --- fmt-conversion.c    (revision 34498)
> +++ fmt-conversion.c    (working copy)
> @@ -57,6 +57,8 @@
>     {IMGFMT_RGB8,    PIX_FMT_BGR8},
>     {IMGFMT_RGB4,    PIX_FMT_BGR4},
>     {IMGFMT_BGR8,    PIX_FMT_PAL8},
> +// NB: This works only because PIX_FMT_0RGB32 is a CPP Macro.
> +//     It wouldn't if it was an enum

I'd say something like "note that most other PIX_FMT values are enums" for the second part.
But just commit something, the main point is to make people stop and think, it doesn't have to be perfect.