Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[client API] Too many logs make ring buffer run out #838

Closed
bylee20 opened this issue Jun 6, 2014 · 11 comments
Closed

[client API] Too many logs make ring buffer run out #838

bylee20 opened this issue Jun 6, 2014 · 11 comments

Comments

@bylee20
Copy link
Contributor

bylee20 commented Jun 6, 2014

When opening one of my sample mkv file with "-v", I got next logs:

[mkv] /---- [ parsing seek head ] ---------
[mkv] Element 0x1f43b675 at 5642.
...
[mkv] Element 0x1f43b675 at 486829844.
[mkv] \---- [ parsing seek head ] ---------

The problem is that the number of lines is over 1400.
This causes running out of ring buffer for client API and makes the application crashed.
Since all the parsed head informations are logged at once in demux_mkv_read_seekhead(), it's not a problem of lazy pulling of log events from client side.
Is there any way to avoid runnig out of message buffer in this case? Or, can this be fixed in mpv's client API?

@ghost
Copy link

ghost commented Jun 6, 2014

The ring buffer for log messages is intentionally fixed size, because otherwise it could happen that we might run out of memory just because a client doesn't read the log messages. (That's also why the per-client event buffer is limited.)

I see the following solutions:

  • bumping the log level of the mkv stuff, since that is usually not really needed anyway
  • making the logbuffer larger (how much?)
  • fixing notifications (as of now, a log message doesn't cause mpv_wait_event() to return; it's polling), and hope the messages are read quickly enough by the client

@bylee20
Copy link
Contributor Author

bylee20 commented Jun 6, 2014

For a debugging purpose, anyone may want to retrieve all logs and in that case, the first option cannot be a solution.
Also, because no one can expect the large enough size, the second option is not desirable.
Maybe change of notifocatio is best like the third option...
Or, how about callback for logging? I know you don't like callbacks but this is the most simple and flexible solution for client developers.

@ghost
Copy link

ghost commented Jun 6, 2014

OK, I did two things:

  • the log buffer is now much larger if the log level is at least verbose
  • the wakeup callback is now called every time a new message is added (but the wakeup pipe is unaffected)
    Is this sufficient?

I also have 2 alternative ideas that could be implemented:

  • make logging independent of mpv_wait_event(), so that the API user can directly wait for new messages
  • allow all logging to be written to a unix FD (which could point to a file or a pipe), using the same formatting that's usually used for the terminal

@bylee20
Copy link
Contributor Author

bylee20 commented Jun 7, 2014

Yep, it's sufficient for now. Thank you!
For the alternatives, can the pipe be used on windows, too?
Otherwise, I think the first option is better.

@ghost
Copy link

ghost commented Jun 7, 2014

For the alternatives, can the pipe be used on windows, too?

No. Windows hates pipes.

I'm hesitant with the first option - it would add more (and more complicated) API, so if the current solution works sufficiently, I'll leave it at this for now.

@bylee20
Copy link
Contributor Author

bylee20 commented Jun 7, 2014

Okay, I'll bring up this issue later when the problem appears again. Thanks!

@bylee20 bylee20 closed this as completed Jun 7, 2014
@bylee20 bylee20 reopened this Jan 13, 2015
@bylee20
Copy link
Contributor Author

bylee20 commented Jan 13, 2015

I think it's time to bring up the old issue.

I have a file which produces next logs in a certain point:

[ffmpeg/video] mpeg4: ac-tex damaged at 11 1
[ffmpeg/video] mpeg4: Error at MB: 50
[ffmpeg/video] mpeg4: marker does not match f_code
[ffmpeg/video] mpeg4: marker does not match f_code
[ffmpeg/video] mpeg4: marker does not match f_code
[ffmpeg/video] mpeg4: marker does not match f_code
[ffmpeg/video] mpeg4: marker does not match f_code
... (repeat the last line)

I guess this is the same issue with this because it crashes when creating mpv_event_log_message in mpv_wait_event().

This time, the whole number of log lines is over 27000.
If you don't think it's not the issue of long log and you need sample, please let me know.

@ghost
Copy link

ghost commented Jan 13, 2015

It certainly shouldn't crash. A sample would probably be helpful to reproduce this.

@bylee20
Copy link
Contributor Author

bylee20 commented Jan 13, 2015

I cut some part of front from original file. The problem occurs around 1m 43s.

ghost pushed a commit that referenced this issue Jan 13, 2015
It just crashed. The prefix and text fields point to static strings in
this case. Oops.

Fixes the issue mentioned in #838.
@ghost
Copy link

ghost commented Jan 13, 2015

The sample was helpful, thanks. Fixed.

@bylee20
Copy link
Contributor Author

bylee20 commented Jan 14, 2015

It works. Thank you! (link removed)

@bylee20 bylee20 closed this as completed Jan 14, 2015
ghost pushed a commit that referenced this issue Jan 24, 2015
It just crashed. The prefix and text fields point to static strings in
this case. Oops.

Fixes the issue mentioned in #838.
ghost pushed a commit that referenced this issue Jan 25, 2015
It just crashed. The prefix and text fields point to static strings in
this case. Oops.

Fixes the issue mentioned in #838.
olifre pushed a commit to olifre/mpv that referenced this issue Feb 28, 2015
It just crashed. The prefix and text fields point to static strings in
this case. Oops.

Fixes the issue mentioned in mpv-player#838.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant