[FFmpeg-devel] [PATCH 0/4] ffplay and lavd SDL2 set

[Thomas Volkert]

On 15.09.2016 08:55, Josh de Kock wrote:
> On 14/09/2016 23:44, Thomas Volkert wrote:
>> On 15.09.2016 00:27, Josh de Kock wrote:
>>> Hi,
>>>
>>> Resending this set with ffplay now having two versions, a SDL2 and a
>>> SDL1 version. I've integrated all comments up until now (hopefully).
>>>
>>> Josh
>>>
>>> Josh de Kock (3):
>>>    lavd: Add SDL2 output device
>>>    ffplay: make copy for SDL1
>>>    MAINTAINERS: update my entries
>>>
>>> Marton Balint (1):
>>>    ffplay: add SDL2 support
>>>
>>>   MAINTAINERS               |   4 +-
>>>   Makefile                  |   1 +
>>>   configure                 |  35 ++-
>>>   ffplay.c                  | 599
>>> +++++++++++++++++++---------------------------
>>>   ffplay.c => ffplay_sdl1.c |   1 -
>>>
>>
>> Maintaining two versions of ffplay sounds as too much overhead to me. Is
>> it not possible to add some more code abstraction or even some simple
>> #ifdef constructs to support SDL1 as well as SDL2 in only one ffplay
>> version in parallel?
>> So, the actually used SDL version could be selected during the configure
>> step. After some months, the (hopefully) deprecated SDL1 code could be
>> dropped...
>>
>
> Yes, it is quite a bit of unnecessary overhead, but it's much cleaner 
> than #ifdeffery in a single source file. 

In general, I don't like the idea of having 2 source files with almost 
the same content. This usually indicates to me that the code structure 
could be improved.
But ...you said you take over the maintenance which is needed. So, I am 
fine with this step as long as the old file gets dropped soon (in terms 
of months instead of years).
(And I also don't like a flood of #ifdefs. ;) )

> Also, to change ffplay.c for most fixes as you'd have to reflect the 
> change on either side of the #ifdefs, so you don't gain anything from 
> not splitting the sources.

I agree for SDL related patches. This apparently has to be done for SDL1 
as well as SDL2.
However, for other patches you have to do copy+paste to get them in both 
ffplay source files.
But see above .. I am fine if you compensate this.

Best regards,
Thomas.