Skip to content
New issue

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

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

Already on GitHub? # to your account

Unable to unset demuxer-lavf-format #15840

Closed
6 tasks done
sodface opened this issue Feb 8, 2025 · 4 comments
Closed
6 tasks done

Unable to unset demuxer-lavf-format #15840

sodface opened this issue Feb 8, 2025 · 4 comments
Labels

Comments

@sodface
Copy link

sodface commented Feb 8, 2025

mpv Information

mpv 0.39.0 Copyright © 2000-2024 mpv/MPlayer/mplayer2 projects
libplacebo version: v6.338.2
FFmpeg version: 6.1.2
FFmpeg library versions:
   libavcodec      60.31.102
   libavdevice     60.3.100
   libavfilter     9.12.100
   libavformat     60.16.100
   libavutil       58.29.100
   libswresample   4.12.100
   libswscale      7.5.100

Other Information

- Linux version: Alpine Linux v3.21
- Kernel Version: 6.12.9-0-lts
- GPU Model: Intel Corporation Iris Plus Graphics G4 (Ice Lake) (rev 07)
- Mesa/GPU Driver Version: mesa-24.2.8
- Window Manager and Version:Cagebreak 2.4.0
- Source of mpv: Alpine Repository
- Latest known working version: 0.38.0
- Issue started after the following happened: upgrade to 0.39.0

Reproduction Steps

Start mpv with:

 mpv --load-scripts=no --script=$(pwd)/mpv-sb.so --idle

mpv-sb.so is a work in progress c-plugin that sets demuxer-lavf-format based on header data sent by a media server. For files that the media server doesn't natively support, I need to unset demuxer-lavf-format and let mpv probe for the format to use. The below c-plugin code worked with mpv 0.38.0, allowing me to play a list of tracks and setting or unsetting demuxer-lavf-format as required for the track.

char *fmt;

 switch (msg.strm.mode) {
  case 'a': fmt = "aac"; break;
  case 'd': fmt = "dsf"; break;
  case 'f': fmt = "flac"; break;
  case 'm': fmt = "mp3"; break;
  case 'o': fmt = "ogg"; break;
  case 's': fmt = NULL; break;
  case 'u': fmt = "opus"; break;
  default: fmt = NULL;
}

mpv_set_property(g_handle, "demuxer-lavf-format", MPV_FORMAT_STRING, &fmt);

After playing a track that explicitly sets the format and then changing to a track that doesn't, which sets fmt = NULL, I now get:

[lavf] Unknown lavf format 
[lavf] Unknown lavf format 
A: 00:00:03 / 00:05:00 (1%)
Failed to recognize file format.

It seems that format is always set here now, even after trying to unset it. I tested with this patch and setting fmt = "none" above instead of NULL and that does restore the old behavior:

--- ./demux/demux_lavf.c
+++ ./demux/demux_lavf.c
@@ -448,7 +448,7 @@
         format = s->lavf_type;
     if (!format)
         format = avdevice_format;
-    if (format) {
+    if (format && strcmp(format, "none") != 0) {
         if (strcmp(format, "help") == 0) {
             list_formats(demuxer);
             return -1;​

I did a git bisect between 0.38.0 and 0.39.0 and this is the commit that broke it for me:

5edc8973ebdb2f80e1fa5b9c7b0ea7d12fd2fe3b is the first bad commit
commit 5edc8973ebdb2f80e1fa5b9c7b0ea7d12fd2fe3b (HEAD)
Author: Kacper Michajłow <kasper93@gmail.com>
Date:   Thu Sep 5 22:14:28 2024 +0200

    various: use talloc_replace

 input/input.c              | 3 +--
 options/m_option.c         | 6 ++----
 player/osd.c               | 9 +++------
 video/out/vo_gpu_next.c    | 6 ++----
 video/out/wayland_common.c | 4 +---
 video/out/x11_common.c     | 3 +--
 6 files changed, 10 insertions(+), 21 deletions(-)

I reverted the change to options/m_option.c and that fixes it for me.

Is there something I should do differently to unset "demuxer-lavf-format" rather than setting it to NULL?

Expected Behavior

Set and unset demuxer-lavf-format successfully.

Actual Behavior

Unable to unset demuxer-lavf-format after setting it.

Log File

mpv.log

I know that's not a proper log file. When I run with --log-file I get:

Assertion failed: str (../misc/json.c: write_json_str: 258)

Sample Files

No response

I carefully read all instruction and confirm that I did the following:

  • I tested with the latest mpv version to validate that the issue is not already fixed.
  • I provided all required information including system and mpv version.
  • I produced the log file with the exact same set of files, parameters, and conditions used in "Reproduction Steps", with the addition of --log-file=output.txt.
  • I produced the log file while the behaviors described in "Actual Behavior" were actively observed.
  • I attached the full, untruncated log file.
  • I attached the backtrace in the case of a crash.
@na-na-hi
Copy link
Contributor

na-na-hi commented Feb 8, 2025

MPV_FORMAT_STRING API doc says:

* NULL isn't an allowed value.

@kasper93
Copy link
Contributor

kasper93 commented Feb 8, 2025

It seems that format is always set here now, even after trying to unset it. I tested with this patch and setting fmt = "none" above instead of NULL and that does restore the old behavior:

I think we should just check if string is not empty.

Like na-na-hi said, NULL isn't really useful to be set to string options. I know it is internally NULL by default, but neither js, lua or cli allows us to set NULL value. And it was working only in C by sheer accident how strings were copied. Now indeed, the string is left in empty state, which is incidentally what you would get anyway on clean mpv instance if you do mpv_get_property.

Also as you noticed our logging asserts out on NULL values when it tries to format it. So really, that's not a valid usage of this API.

@sodface
Copy link
Author

sodface commented Feb 9, 2025

Thank you both for the replies, indeed, I missed the comment in client.h. I'm still a little unclear on whether there's a method to unset demuxer-lavf-format once it's been set, without restarting mpv? There was in 0.38.0 (accidentally through my misuse of the api) and now in 0.39.0 that no longer works. I can see that's not technically a regression, still it was nice that it worked and it's sort of critical to the functionality of the plugin I'm working on. Is support for this something that might be considered or do I need to try to figure out another approach?

@kasper93
Copy link
Contributor

kasper93 commented Feb 9, 2025

Yes, I will make it possible.

kasper93 added a commit to kasper93/mpv that referenced this issue Feb 9, 2025
mpv internally treats all string options/properties with NULL or an empty string the same way. client.h explicitly forbids MPV_FORMAT_STRING from being NULL, but in the C API, it has been working accidentally due to how strings are copied. Nevertheless, none of the APIs allow this; JavaScript, Lua, IPC, and CLI all require strings to be at least empty. We cannot pass NULL.

Furthermore, currently passing NULL causes an assertion failure in the JSON formatter, so it is clearly not intended to be used that way.

Internally, all string options default to NULL, but in this case, they should behave exactly the same as an empty string. Hence, this change is applied to the `demuxer-lavf-format` option.

Note that get_property will never return a NULL value, regardless of the internal value.

Fixes: mpv-player#15840
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants