[MPlayer-cvslog] r32478 - in trunk: DOCS/tech/slave.txt command.c input/input.c input/input.h

[Reimar Döffinger]

On Tue, Oct 12, 2010 at 12:00:26PM +0200, Nicolas George wrote:
> Le nonidi 19 vendémiaire, an CCXIX, Reimar Döffinger a écrit :
> > Uh, this was never reviewed.
> 
> The first version got a few remarks from Diego that I took into account.

Usually you shouldn't expect Diego to comment much on code correctness,
you'd have to get someone else to review for that (unfortunately
that usually means you'll have to get me to review).

> In the current version, the list of EOSD sources is not emptied between
> files. It could be, I have no strong feelings either way. Keeping it
> requires slightly simpler code, while emptying it is maybe slightly more
> logical.

Actually keeping it is nicer for anything that is not file-specific
subtitles.
Strictly speaking this is something that might make sense as a
per-source property, at least conceptually it would make sense
to remove the ASS source, whereas for any kind of overlay 
(e.g. a kind of newsticker) it makes more sense to keep.

> > This will cause a warning on 64 bit.
> 
> Patch attached. Either fixing these two or also fixing the six ones that
> were already there.

I don't like the double-casts, but I'd say apply the patch fixing all.

> > Useless {}
> 
> I imitated from the two cases just above. Shall I remove only my two pairs,
> or all four?

I realize it's an inconsistent mess anyway, but I'd say remove those four.

> Le nonidi 19 vendémiaire, an CCXIX, Reimar Döffinger a écrit :
> > I extracted the code in gl_common into a separate function,
> > it just needs to go into some appropriate file.
> 
> I saw the commit, thanks.
> 
> Any preference about the place for the appropriate file? You recently said
> that you did not like to have everything in the root directory. Maybe
> misc_util/pnm_loader.c?

IMO please don't introduce such meaningless names as misc_util.
Just put it in root for now if you want.
I just think that everything related to subs, OSD and EOSD really
needs to be moved out of the root dir, it is too much.
My suggestion is to name that dir sub/, because all these are
related to subtitles (even though they are about more than subtitles).

> bstr.*, mpbswap.h, unrar_exec.*, gui/bitmap.*, libmpcodecs/cmmx.h could
> possibly go there too.

Except mpbswap.h I don't think any of those are likely to have more than
a single user, and as such should stay with the file that uses it.
cmmx.h is definitely deprectated, and I doubt much if any of the gui/* code
is something where reusing the code should be encouraged, at least not
without a previous review.