[MPlayer-dev-eng] [PATCH] Subtitles directories

[Reimar Döffinger]

On 21 dec 2010, at 11:00, Clément Bœsch <ubitux at gmail.com> wrote:

> On Thu, Dec 16, 2010 at 09:58:39PM +0100, Clément Bœsch wrote:
>> On Mon, Nov 29, 2010 at 11:08:55PM +0100, Clément Bœsch wrote:
>>> On Tue, Nov 23, 2010 at 08:07:25AM +0100, Reimar Döffinger wrote:
>>>> On Tue, Nov 23, 2010 at 12:50:45AM +0100, Clément Bœsch wrote:
>>>>> Oh. This is bad. How do you want me to handle that? Of course I personally
>>>>> consider the factorization also being a fix for the MEncoder code, but
>>>>> something tell me you won't agree with that :-(
>>>> 
>>>> I am ok with considering a fix, I am just not convinced it is fixing it
>>>> the right way round. But it probably is more flexible this way.
>>>> 
>>>>> By the way, is the rest of this first patch clear enough now?
>>>> 
>>>> I didn't look that closely yet. I still think it is too much code,
>>>> but that is just a gut feeling and may be wrong of course.
>>> 
>>> Sorry I couldn't simplify it more. Still not good to commit?
>>> 
>>> Also, I noticed the VOB-Sub loading is totally different from the default
>>> one. I'd like to work on it to make it benefit from other subtitles
>>> options (like sub-fuzziness, sub-directories too, etc); I'd like to get
>>> rid of the hackish loading in mplayer.c. I really can't integrate the
>>> subdirs option for vobsub in the current state without huge changes (and I
>>> don't think you want that, neither I do since it will delay the patch
>>> again).
>>> 
>> 
>> Well, since I still don't have any feedback, I tried to split it more. So
>> There is now 3 patches. I wonder how I can split it more. Please review
>> this so I can move on :)
>> 
> 
> [...]
> 
>> Subject: [PATCH 1/3] Factorize subtitles loading between mplayer and mencoder.
>> 
>> ---
>> mencoder.c      |   18 +-----------------
>> mplayer.c       |   16 +---------------
>> sub/subreader.c |   27 ++++++++++++++++++++++++++-
>> sub/subreader.h |    2 +-
>> 4 files changed, 29 insertions(+), 34 deletions(-)
>> 
> 
> I will commit this first patch in the next three days so we can move on.

You removed the filename == NULL check that was in the mencoder code, either keep that or double and triple check the code.
Otherwise it seems ok to me, did not yet properly check the other patches.