Le quartidi 14 pluviôse, an CCXIX, Reimar Döffinger a écrit :

Adding a dependency on the subreader isn't that great

Inside mplayer, the subreader is included inconditionnally. I suppose you are thinking about the effort to make mpcodecs a stand-alone library?

and the pts you have in the video filter is not at all realiable.

Are you sure about that? I think I read exactly the opposite. In particular, I believe that ASS chose to use it because it was deemed _more_ reliable than the global PTS, especially if there are framerate-modifying filters.

I consider it likely that a custom format using frame numbers would work better in several aspects and probably even need less code.

I really do not like the idea of a custom format: not only does it make new code, and therefore new bugs, but also it prevents the users from using existing tools to manipulate the script. There are a lot of editors for text subtitles, there would not be for a custom format. If we want to avoid depending from the subreader in the video filters, I can see another, more generic solution: - make vf_delogo accept a control message to change the rectangle; - add the corresponding slave command; - create a system for time-based scripted slave commands. This is more work, but I think it may be worth it. What do you think about it? Regards, -- Nicolas George

Le quintidi 15 pluviôse, an CCXIX, Reimar Döffinger a écrit :

To my knowledge the time stamps used here do reset and wrap-around, and the subtitle code does not really work well for that.

I would rather fix this timestamp so that it is reliable. That seems like a better solution on the long-term. If someone has samples where the global timestamp is correct but the video filter timestamp is not, please let me know.

Which is why I suggested frame numbers, that is easy to make switch the location at exactly the desired time, however it does not work together with seeking.

Working with seeking is, IMHO, a requisite.

The reason for the suggestion is that it should be less new code than (mis-)using the subreader.

Ok, I did that. I am not very happy with it, it is less robust with more code, but it is self-contained.

Though there's even the question if there's a need to use a file or if it couldn't just be passed as filter argument.

The situation I wrote it for is to remove badly synced hardcoded subtitles (this is also the reason for the other patch, the one that dumps the bounding boxes of the subtitles): that makes about four hundred timestamps and rectangles. Technically, it is possible to pass them on the command line on most OS, and I am proficient enough in shell to do it without too much trouble, but this is not very convenient, and not suitable for a publicly available option.

If anything it should be a generic command to change the filter arguments, as implement for audio filters.

It also won't work with mencoder, and I have some doubts about the time-based stuff, MPlayer does not have a time-base that will make everyone happy.

Indeed, especially with the risk of an A-V desync. Here is a new version, with a custom format parser. Regards, -- Nicolas George

Le primidi 21 pluviôse, an CCXIX, Reimar Döffinger a écrit :

const char * is probably better.

Changed.

doxy-documentation for new functions would be nice.

Added.

Seems to me liek you could just do this in fix_band

Indeed.

Are you sure you want to allow it to go backwards?

Yes. The user may have just hit the left arrow to go back a few seconds.

This looks a bit double...

Fixed.

...

+ rect = realloc(rect, alloc_rect * sizeof(*rect)); Potential integer overflow.

Indeed. What is the solution in that case? if (alloc_rect * sizeof(*rect) / sizeof(*rect) != alloc_rect) abort(); ? There are a lot of similar problems all around the code.

...

+ if (!rect) + abort(); Abort sure isn't nice. Haven't looked what if any the alternatives are.

I looked around before writing this code. There is no consistent behaviour about this: there are a lot of unchecked mallocs, error returns which will be later ignored, etc. On current operating systems, mermory is overcommitted and small mallocs never fail, so trying to recover gracefully on malloc failure in these cases is mostly wasted time. I think what we need is something like mp_realloc_or_die that takes care both of improbable failure and integer overflow.

I'd expect this whole part can be simplified by fscanf, in the worst case by also using getc/ungetc

I do not see it getting simpler than that.

...

+ rect = realloc(rect, n_rect * sizeof(*rect)); That's just pointless just to save a few bytes IMO.

I find it sloppy not to do it, as it is just a simple line like that. Furthermore, it would allow valgrind to check out of bounds accesses later for example. Again, mp_enlarge_array / mp_finish_array would probably be nice for similar cases, which are quite common. Regards, -- Nicolas George

Le duodi 22 pluviôse, an CCXIX, Reimar Döffinger a écrit :

if (alloc_rect > INT_MAX / sizeof(*rect))

Always keep the value you need to validate on its own, otherwise it gets really difficult to make sure you didn't miss anything (in this case it also has the very minor advantage of not requiring a division at runtime, it can be calculated at compile-time). Since you always multiply by two other checks (like checking that previous_n * sizeof() < INT_MAX/2) are possible as well.

I did not find struct_realloc, but it does not take care of dying anyway. Here is a new version that introduce *alloc functions that print an error message and exit if they fail (keep in mind that most current OS overcommit the memory, so the mallocs usually do not fail anyway). They could be more widely used in the rest of the code.

I always forget what exactly sscanf supports, but adding a \n or a space at the end should help catch some wrong formats e.g. when it ends with "12something"

Not so simple, sscanf would just stop parsing, and since there are no more conversion, it does not report it. Anyway, it is not a serious problem to parse "12something" as "12". In fact, it makes the parser support comments there although it was not supposed to. Regards, -- Nicolas George

Le septidi 27 ventôse, an CCXIX, Ingo Brückl a écrit :

...

+static void mp_die(void) +{ + abort(); +} + The GUI would need a call to something like guiDie() there before.

Would something like that suit you: static void mp_die_default(void) { abort(); } void (*mp_die)(void) = mp_die_default; And what do other think of it? Remember that this kind of die is for catastrophic failures; you can not expect to be able to open a dialog box, for example. Regards, -- Nicolas George

L'octidi 28 ventôse, an CCXIX, Ingo Brückl a écrit :

if (use_gui) guiDie(); // or guiAbort()

doYouReallyInsistOnTheCamelCase? iFindItRatherUgly.

I was simply thinking of something like and caring about this CONFIG_GUI later together with all the other ones.

I'd rather avoiding more ifdefs if a cleaner solution can be found immediately. In this case, overriding the whole body of a function, the solution of the function pointer seems quite easy and clean. (Another solution would be tu put mp_die in a separate file and link with a different version for the GUI, but that is more work.)

I'd like to receive the MSGL_FATAL messages and try to inform the GUI user about these.

In this particular case, it may not be possible, as the message is about a malloc failure. But anyway.

The guiDie() (or guiAbort()) should clean up and destroy the GUI (if established).

Is there really something to do? Usually, if the application dies suddenly, any GUI it has opened just disappears, and that is fine. Regards, -- Nicolas George

On Wed, Mar 16, 2011 at 07:49:57PM +0100, Nicolas George wrote:

Le duodi 22 pluviôse, an CCXIX, Reimar Döffinger a écrit :

...

if (alloc_rect > INT_MAX / sizeof(*rect))

Always keep the value you need to validate on its own, otherwise it gets really difficult to make sure you didn't miss anything (in this case it also has the very minor advantage of not requiring a division at runtime, it can be calculated at compile-time). Since you always multiply by two other checks (like checking that previous_n * sizeof() < INT_MAX/2) are possible as well.

I did not find struct_realloc, but it does not take care of dying anyway.

libmpdemux/demuxer.h

Here is a new version that introduce *alloc functions that print an error message and exit if they fail (keep in mind that most current OS overcommit the memory, so the mallocs usually do not fail anyway). They could be more widely used in the rest of the code.

I don't understand what the fascination with those hard aborts are. It won't quite properly, it might leave behind gigantic core dumps causing issues due to the file system filling up, the terminal restoration code is there but I think it still uses non-signal safe functions and thus is not reliable at all, when encoding the file header won't be written thus possibly leaving behind a completely useless file even if it happens on e.g. the last black frame nobody cares about anyway. Yes, some of these do not apply here, and not failing promptly is ugly here since this is basically an encode-only feature, but still I just can't see how aborting would be a good idea ever (except that it might exploitable errors less likely or harder to exploit, but it doesn't seem that great for that either).

Le duodi 22 germinal, an CCXIX, Nicolas George a écrit :

Subject: [PATCH 1/3] Introduce the exit_player function in mencoder. Subject: [PATCH 2/3] Introduce mp_mem.[ch] for memory alloc helpers. Subject: [PATCH 3/3] vf_delogo: allow to change the rectangle based on the time

Ping? Regards, -- Nicolas George

On 17 May 2011, at 16:29, Nicolas George <nicolas.george@normalesup.org> wrote:

Le duodi 22 floréal, an CCXIX, Nicolas George a écrit :

...

...

Subject: [PATCH 1/3] Introduce the exit_player function in mencoder. Subject: [PATCH 2/3] Introduce mp_mem.[ch] for memory alloc helpers. Subject: [PATCH 3/3] vf_delogo: allow to change the rectangle based on the time Ping?

Well, this is IMHO a very useful feature, I'd like to commit soon, if no one objects.

I still dislike the mp_mem all-around: I do not like those hard aborts, I do not like adding a new file with "generic" code that will be used only in one place (with a risk that this never changes, making it a useless maintenance burden) and I don't really like that the realloc integer overflow check is not done in the usual way (nelem > MAX / element_size), even though I at least currently can't think of a case where it would fail.

-- Nicolas George _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng

On 18 May 2011, at 13:16, Nicolas George <nicolas.george@normalesup.org> wrote:

Le nonidi 29 floréal, an CCXIX, Reimar Döffinger a écrit :

...

I still dislike the mp_mem all-around: I do not like those hard aborts, I

Maybe you did not look at the latest version: there are no longer hard aborts but rather clean shutdowns using exit_mplayer().

I considered exit_mplayer a "hard abort". And I can't quite decide whether it is better or worse: If someone ever used the function in code that somehow might be called by exit_mplayer you can now get an endless recursion.

...

do not like adding a new file with "generic" code that will be used only in one place (with a risk that this never changes, making it a useless maintenance burden)

I agree this is not ideal, but I believe that adding the code to do the error checks as is in place is much worse: we end up with each place needing malloc doing things its ways, sometimes with size_t and sometimes with int, sometimes checking for overflow, sometimes only for failure, and sometimes for neither, etc.

I already said that I am quite in favour of code like realloc_struct handling overflows. However the failure handling is typical dead-end code: though in principle sensible, it is still a lazy hack - good code would return error codes up the chain until a point where the error can be handled, e.g. by just going on to play the next file - and "good code" will not be able to use a function with built-in exit.

...

and I don't really like that the realloc integer overflow check is not done in the usual way (nelem > MAX / element_size), even though I at least currently can't think of a case where it would fail.

I am pretty sure this version is correct,

Usually (theoretical) issues come up once signed types become involved, haven't checked for that.

and I quite like the fact that it works even if SIZE_MAX is missing or wrong or if the type of the variable changes,

Some arbitrary value like INT_MAX (or rather some fixed constant that will always fit in an int) also has the advantage that behaviour will be the same on all systems. But either way I think there has been enough pointless discussion. I sure think that reusing the existing function is better (since it is tested and has several users), and that exiting on failure has no real advantages but has several issues including discouraging really good code but if I can't convince you just go ahead.

On Sun, May 22, 2011 at 10:10:37AM +0200, Nicolas George wrote:

Le decadi 30 floréal, an CCXIX, Reimar Döffinger a écrit :

...

I considered exit_mplayer a "hard abort". And I can't quite decide whether it is better or worse: If someone ever used the function in code that somehow might be called by exit_mplayer you can now get an endless recursion.

This is a valid concern, indeed.

...

Some arbitrary value like INT_MAX (or rather some fixed constant that will always fit in an int) also has the advantage that behaviour will be the same on all systems. But either way I think there has been enough pointless discussion. I sure think that reusing the existing function is better (since it is tested and has several users), and that exiting on failure has no real advantages but has several issues including discouraging really good code but if I can't convince you just go ahead.

I rewrote the delogo patch without relying on new utility functions. Do you like it better that way?

Yes, I'd say go ahead. But I'd like to say that despite the criticism I do appreciate your efforts to improve things. For example I also do agree that the struct_realloc function is useful and the current name and location is really crappy. Though I also think the most straight-forward way to get things moving (and in the process fully understand the requirements and issues) is to just extract common code with as little changes as possible as a first step.