loadfile: update the format of terminal track information #14405

guidocella · 2024-06-21T19:14:10Z

Stop making unselected tracks and editions grey because they can be hard to read over a dark background (\033[2m would be hard to differentiate from regular text with a light theme instead), and because there is no way to not print the escape sequences in --log-file.

Just use the same circles as the OSD and OSC. We need to print the empty circles for alignment on mlterm with East Asian fonts (we could also make them invisible with \033[8m but it would still get added to log files).

Add back the space before tracks and editions so they look like a sub-section of the "Playing..." line printed with playlists and of "Track switched", consistently with the metadata which starts with space which makes it look like a sub-section of the "File tags" line.

Also stop converting Hz to kHz for consistency with other log messages, e.g. AO: [pipewire] 48000Hz stereo 2ch floatp

sfan5

I prefer "2ch 48000Hz" but this addresses the important points
"2.0 48000Hz" is also okay but not used inside mpv otherwise

sfan5 · 2024-06-21T19:39:09Z

relevant PR #14192 (comment)

github-actions · 2024-06-21T19:42:36Z

Download the artifacts for this pull request:

Windows

macOS

player/loadfile.c

guidocella · 2024-06-21T22:36:04Z

Do we want to merge the track data in one set of parentheses, or at least hls-bitrate and external?

rach-md · 2024-06-22T01:46:25Z

Could you make it more consistent? samplerate on ao does not use space but track does. Also codec profile only show up after track switched, so show them from very first or just remove it.

kasper93 · 2024-06-23T12:13:19Z

editions are still quoted, while titles are not
after consideration editions should be first as before, they are the biggest, most important entity
there are two spaces after ●/○ for edition, while there are no for video/audio/subs
two spaces after track title look like an error, the separation is no clear enough and looks strange. Either bring back ' or make the separation more pronounced by other means
I think I preferred track modificators at the beginning, they are mixed in between the title and parameters now, maybe at the end would be better, after params.
For edition to be consistent we could alias edition to eid and do [ 0.035914] cplayer: ● Edition --eid=0 'Director's Cut' (*) aligned with the rest.

[  0.035624]    cplayer: ● Video  --vid=1  --vlang=eng  Salt (2010)  (*) (h264 1920x800 23.976 fps)
[  0.035679]    cplayer: ● Audio  --aid=1  --alang=eng  AC3 5.1 @ 640 Kbps  (*) (ac3 6ch 48000 Hz)
[  0.035722]    cplayer: ○ Audio  --aid=2  --alang=pol  AC3 5.1 @ 640 Kbps  (ac3 6ch 48000 Hz)
[  0.035761]    cplayer: ○ Audio  --aid=3  --alang=eng  Commentary  (aac 2ch 48000 Hz)
[  0.035810]    cplayer: ○ Subs   --sid=1  --slang=pol  (*) (subrip)
[  0.035846]    cplayer: ● Subs   --sid=2  --slang=eng  (subrip)
[  0.035881]    cplayer: ○ Subs   --sid=3  --slang=eng  forced  (subrip)
[  0.035914]    cplayer: ●  --edition=0 'Director's Cut' (*)
[  0.035947]    cplayer: ○  --edition=1 'Extended Cut'

guidocella · 2024-06-23T12:29:07Z

Updated. Editions had issues because I've been modifying them blindly since I can't find any file with them so I don't think it's worth adding an option alias just for aligning them with tracks when they're so rare.

kasper93 · 2024-06-23T13:25:45Z

Looks like this now

[  0.035891]    cplayer: ● --edition=0  'Director's Cut'  (*)
[  0.035935]    cplayer: ○ --edition=1  'Extended Cut'
[  0.035994]    cplayer: ● Video  --vid=1  --vlang=eng  'Salt (2010)' (h264 1920x800 23.976 fps) (*)
[  0.036032]    cplayer: ● Audio  --aid=1  --alang=eng  'AC3 5.1 @ 640 Kbps' (ac3 6ch 48000 Hz) (*)
[  0.036073]    cplayer: ○ Audio  --aid=2  --alang=pol  'AC3 5.1 @ 640 Kbps' (ac3 6ch 48000 Hz)
[  0.036106]    cplayer: ○ Audio  --aid=3  --alang=eng  'Commentary' (aac 2ch 48000 Hz)
[  0.036139]    cplayer: ○ Subs   --sid=1  --slang=pol  (subrip) (*)
[  0.036172]    cplayer: ● Subs   --sid=2  --slang=eng  (subrip)
[  0.036204]    cplayer: ○ Subs   --sid=3  --slang=eng  'forced' (subrip)

I'm fine with this. Let see what @sfan5 thinks though.

guidocella · 2024-06-23T13:32:17Z

Maybe use single spaces for editions?

kasper93 · 2024-06-23T13:41:56Z

Maybe use single spaces for editions?

Ok for me.

player/loadfile.c

kasper93 · 2024-06-23T15:45:21Z

player/loadfile.c

        APPEND(b, " (external)");
+    if (t->default_track)
+        APPEND(b, " (*)");
+    if (t->forced_track)
+        APPEND(b, " (f)");
+    if (t->attached_picture)
+        APPEND(b, " [P]");


Also instead of hieroglyphs, we could print it in full. Bit longer, but who know what [P] means?

(default, external, picture) for example, in most cases there are 1-2 flags per track.

I think short form is fine for (*) because it's so common

It is incomprehensible without reading the code though

We can simply document it in the manual

default is not that long and there is plenty of space (excluding title, which may not fit anyway). I mean sure, we can make users read code/docs to see what (*) means, but how to you even search for this? And why explain output when we can make it self-explainable? No other media player does some cryptic flags.

I agree with kasper, I don't see where you could even put in the docs.

sfan5 · 2024-06-23T16:24:14Z

player/loadfile.c

-                print_stream(mpctx, mpctx->tracks[n]);
+                print_stream(mpctx, mpctx->tracks[n], msg ||
+                             mpctx->playlist->num_entries > 1 ||
+                             mpctx->playing->playlist_path);


a comment would be good

player/loadfile.c

kasper93 · 2024-06-23T17:25:13Z

player/loadfile.c

+    if (t->default_track || t->forced_track || t->attached_picture ||
+        t->visual_impaired_track || t->hearing_impaired_track || t->is_external)
+        APPEND(b, " [");


Suggested change

if (t->default_track || t->forced_track || t->attached_picture ||

t->visual_impaired_track || t->hearing_impaired_track || t->is_external)

APPEND(b, " [");

if (t->default_track || t->forced_track || t->attached_picture ||

t->visual_impaired_track || t->hearing_impaired_track || t->is_external)

{

APPEND(b, " [");

}

or even move it to first check

APPEND(b, first ? " [" : ""); first = false;

sfan5 · 2024-06-23T17:34:13Z

can we maybe not make this into one mega-commit that changes everything all over again?

there are clear problems with the current code that I would prefer to see fixed before the next round of bikesheddings.
(by current I mean what's currently in master since d49879f)

kasper93 · 2024-06-23T17:36:42Z

there are clear problems with the current code

Which ones specifically? I though we are bikeshedding it from the beginning.

guidocella · 2024-06-23T17:46:01Z

Which ones specifically? I though we are bikeshedding it from the beginning.

Same, and the changes are already done.

kasper93 · 2024-06-23T17:47:39Z

LGTM. I'm stopping here. I promise, I won't comment anymore.

kasper93 · 2024-06-23T17:53:20Z

player/loadfile.c

+#define ADD_FLAG(b, flag, first) \
+    APPEND(b, " %s%s", first ? "[" : "", flag); \
+    first = false;


Ok, last one.

Just small suggestion, wrap it in do while, so you don't accidentally forget about {}. And if you do that, you can remove {} from ifs below.

Suggested change

#define ADD_FLAG(b, flag, first) \

APPEND(b, " %s%s", first ? "[" : "", flag); \

first = false;

#define ADD_FLAG(b, flag) do { \

APPEND(b, " %s%s", first ? "[" : "", flag); \

first = false; \

} while(0)

(also removed first arg, we can assume it is there)

sfan5 · 2024-06-23T21:27:57Z

Which ones specifically? I though we are bikeshedding it from the beginning.

The color handling regarding log file and depending on stdout state is objectively broken.

sfan5

Fine otherwise

player/loadfile.c

Stop making unselected tracks and editions grey because they can be hard to read over a dark background (\033[2m would be hard to differentiate from regular text with a light theme instead), and because there is no way to not print the escape sequences in --log-file. Just use the same circles as the OSD and OSC. We need to print the empty circles for alignment on mlterm with East Asian fonts (we could also make them invisible with \033[8m but it would still get added to log files). Add back the space before tracks and editions when printed after "Playing..." or "Track switched" and similar, so they look like a sub-section of it, consistently with the metadata which starts with space which makes it look like a sub-section of the "File tags" line. Leave 2 spaces between track columns. Make the lang options only as long as the longest language. Place hls-bitrate within the same parentheses as the other data. Replace the incomprehensible (*) (f) and [P] with textual descriptions within []. Also place external there. Stop converting Hz to kHz for consistency with other log messages, e.g. AO: [pipewire] 48000Hz stereo 2ch floatp Remove the space in "2 ch" so it doesn't look like 2 separate values (We considered using mp_chmap_to_str(&s->codec->channels) but it prints values like "unknown2").

sfan5 approved these changes Jun 21, 2024

View reviewed changes

guidocella force-pushed the tracks-circle branch from 1246ee9 to 9320d6f Compare June 21, 2024 21:18

sfan5 approved these changes Jun 21, 2024

View reviewed changes

kasper93 reviewed Jun 21, 2024

View reviewed changes

player/loadfile.c Show resolved Hide resolved

kasper93 reviewed Jun 21, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

kasper93 reviewed Jun 21, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from 9320d6f to 46129d8 Compare June 21, 2024 22:09

kasper93 reviewed Jun 21, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

player/loadfile.c Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from 46129d8 to 1eb9177 Compare June 21, 2024 22:25

guidocella force-pushed the tracks-circle branch from 1eb9177 to b427a7a Compare June 23, 2024 12:27

sfan5 reviewed Jun 23, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from b427a7a to 5e355f8 Compare June 23, 2024 15:35

kasper93 reviewed Jun 23, 2024

View reviewed changes

guidocella force-pushed the tracks-circle branch from 5e355f8 to 2743ec1 Compare June 23, 2024 15:56

sfan5 reviewed Jun 23, 2024

View reviewed changes

guidocella force-pushed the tracks-circle branch 3 times, most recently from 067c6f0 to 9eb7f63 Compare June 23, 2024 16:36

kasper93 reviewed Jun 23, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from 9eb7f63 to 8558112 Compare June 23, 2024 16:50

kasper93 reviewed Jun 23, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from 8558112 to 90ac9fe Compare June 23, 2024 16:56

kasper93 reviewed Jun 23, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from 90ac9fe to f39c394 Compare June 23, 2024 17:16

kasper93 reviewed Jun 23, 2024

View reviewed changes

guidocella force-pushed the tracks-circle branch from f39c394 to bf76144 Compare June 23, 2024 17:44

kasper93 approved these changes Jun 23, 2024

View reviewed changes

kasper93 reviewed Jun 23, 2024

View reviewed changes

guidocella force-pushed the tracks-circle branch 2 times, most recently from 20cbfa9 to 50f8db1 Compare June 23, 2024 18:03

sfan5 self-requested a review June 23, 2024 21:40

sfan5 approved these changes Jun 24, 2024

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the tracks-circle branch from 50f8db1 to d5b0d00 Compare June 24, 2024 13:57

sfan5 merged commit 82ffe8f into mpv-player:master Jun 24, 2024
20 checks passed

guidocella deleted the tracks-circle branch June 24, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loadfile: update the format of terminal track information #14405

loadfile: update the format of terminal track information #14405

guidocella commented Jun 21, 2024

sfan5 left a comment

sfan5 commented Jun 21, 2024

github-actions bot commented Jun 21, 2024 •

edited

Loading

guidocella commented Jun 21, 2024

rach-md commented Jun 22, 2024

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

kasper93 commented Jun 23, 2024

kasper93 Jun 23, 2024

sfan5 Jun 23, 2024

guidocella Jun 23, 2024

sfan5 Jun 23, 2024

kasper93 Jun 23, 2024 •

edited

Loading

guidocella Jun 23, 2024

sfan5 Jun 23, 2024

kasper93 Jun 23, 2024

kasper93 Jun 23, 2024

sfan5 commented Jun 23, 2024 •

edited

Loading

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

kasper93 commented Jun 23, 2024

kasper93 Jun 23, 2024 •

edited

Loading

sfan5 commented Jun 23, 2024

sfan5 left a comment

loadfile: update the format of terminal track information #14405

loadfile: update the format of terminal track information #14405

Conversation

guidocella commented Jun 21, 2024

sfan5 left a comment

Choose a reason for hiding this comment

sfan5 commented Jun 21, 2024

github-actions bot commented Jun 21, 2024 • edited Loading

guidocella commented Jun 21, 2024

rach-md commented Jun 22, 2024

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

kasper93 commented Jun 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasper93 Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 commented Jun 23, 2024 • edited Loading

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

kasper93 commented Jun 23, 2024

kasper93 Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

sfan5 commented Jun 23, 2024

sfan5 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 21, 2024 •

edited

Loading

kasper93 Jun 23, 2024 •

edited

Loading

sfan5 commented Jun 23, 2024 •

edited

Loading

kasper93 Jun 23, 2024 •

edited

Loading