Conversation
|
Follow-up on scope, before it comes up in review. The description says no container-tagged file changes behaviour. That holds when the container tag and the in-band signal agree, which is the normal case, but not when they disagree: This is deliberate — it mirrors the frame rotation handling a few lines below in |
|
|
Why are you so rude? Your single accepted PR is larger and contains no technical nothing, it's nothing but bad words on people trying to collaborate because, yes, it's a people dream (humans project into others what they are) to be listed as contributor, not to actually contribute with fixes.... |
? The rude part here is you letting clod flood the maintainers with redundant text. |
Please read the contribution guidelines. You need to show you're capable of responding to reviews with human-written responses, which the AI-slop written commit messages implies you're not. |
AI-slop - define this term, please? What about human slop, like being rude to a genuine contribuition proposal? You are assuming that because I leveraged AI to write the PR is slop? HandBreak actually verifyed my claims before that... This message might contain english errors because it's not my main language and mainteners prefer wrong english than AI assisted writting. |
|
How often the frame-packing SEI is actually attached to a frame? It needs no patched mpv and no sample downloads. The block # Build a 10 s SBS clip with the frame-packing SEI and a keyframe every 48 frames.
ffmpeg -v error -f lavfi -i "testsrc2=size=960x1080:rate=24:duration=10" \
-f lavfi -i "smptebars=size=960x1080:rate=24:duration=10" \
-filter_complex "[0:v][1:v]hstack=inputs=2[v]" -map "[v]" \
-c:v libx264 -crf 20 -pix_fmt yuv420p -g 48 \
-x264-params frame-packing=3 sei_multi.mp4
# Same thing without the SEI, as a negative control.
ffmpeg -v error -f lavfi -i "testsrc2=size=960x1080:rate=24:duration=10" \
-f lavfi -i "smptebars=size=960x1080:rate=24:duration=10" \
-filter_complex "[0:v][1:v]hstack=inputs=2[v]" -map "[v]" \
-c:v libx264 -crf 20 -pix_fmt yuv420p -g 48 no_sei.mp4
# How many frames actually carry AV_FRAME_DATA_STEREO3D?
for f in sei_multi.mp4 no_sei.mp4; do
ffprobe -v error -select_streams v:0 -show_frames "$f" > /tmp/f.$$
printf "%-16s frames=%-5s keyframes=%-4s stereo3d_side_data=%s\n" "$f" \
"$(grep -c '^media_type=video' /tmp/f.$$)" \
"$(grep -c '^key_frame=1' /tmp/f.$$)" \
"$(grep -c '^side_data_type=Stereo 3D' /tmp/f.$$)"
rm -f /tmp/f.$$
done
# And which frames they are.
ffprobe -v error -select_streams v:0 -show_frames sei_multi.mp4 | awk '
/^media_type=video/{n++} /^key_frame=1/{k[n]=1} /^side_data_type=Stereo 3D/{s[n]=1}
END{for(i=1;i<=n;i++) if(k[i]||s[i]) printf "frame %3d keyframe=%d stereo3d=%d\n", i, (k[i]?1:0), (s[i]?1:0)}'OutputSame counts and the same frame indices across three major FFmpeg versions, so the result is a property of the file rather than of one build. Why it is built this way? |
|
Another data point on the approach, in case it is useful to the review: Kodi already does this, and has for years.
Verified by running Kodi 21.2 on a file whose only 3D signal is the SEI (neutral filename, no container tag): Sample to check against: And on why it matters outside players: on 2011–2012 Sony BRAVIA sets this SEI is the only signal that makes the display engage 3D by itself. Measured through two independent DLNA servers, byte-exact: the SEI-carrying file engages 3D, the identical file with the SEI removed plays flat. |
baaf79f to
1c5c04f
Compare
|
Look, I am not that good with words and english can be complicated to convey, I therefore disclosured the leverage of AI on the PR. The new rule also landed one day after I filed the PR and merge request. |
|
@kasper93 urging for a neutral technical review here. |
kasper93
left a comment
kasper93
left a comment
There was a problem hiding this comment.
Thanks, the report is valid and the first commit is the right idea. Keep in mind what mpv does with this value: it places OSD and subtitles per eye for side by side and top bottom layouts, and it is shown in video-params. There is no conversion, so the scope is a lot smaller than the description suggests.
The second commit is workaround for a libavcodec bug, and should not be added here.
What I would like to see:
- Commit 1 with the
opaque_refcomment fixed. - Commit 2 reduced to
if (!m.stereo3d) m.stereo3d = p->codec->stereo_mode;. - The persistence fixed in libavcodec.
ff_h2645_sei_to_frame()already does it for the film grain SEI, frame packing needs the same treatment. - If you want a fallback for older FFmpeg inside mpv, it has to live in vd_lavc and must not be cleared in
reset_params.
Please trim the description and the commit messages to what the code does, we don't need so much of backstory for trivial changes.
1c5c04f to
fc2ac9e
Compare
|
v2 is up, reworked to the review point by point. What changed
Description and commit messages trimmed to what the code does; the LLM-assistance disclosure stays, per contribute.md. Behavior, honestly Precedent Issue impact The fixtures are reproducible in a minute from the ffmpeg commands in the description, if anyone wants to verify the behavior locally; nothing proprietary, no footage. |
fc2ac9e to
3bcee81
Compare
|
Hi from VideoLAN contributor! 🫡 We received the exact same contribution on the VLC side and you'd be surprise to learn the prompt for all those merge requests is partially available on the contributor upstream: https://github.com/danielcamposramos/sony-bravia-linux/blob/main/ai-skill/SKILL.md I've forked it there too:https://github.com/alexandre-janniaux/sony-bravia-linux/blob/main/ai-skill/SKILL.md You'll find such text in the prompt, showing how much this has been automated: Best regards, |
|
The only source of the stereo 3D layout was the Matroska StereoMode element read by demux_mkv. libavcodec attaches the layout signaled in the bitstream, such as the H.264 and HEVC frame packing arrangement SEI, or by the container, such as the MP4 st3d box, to the decoded frame as AV_FRAME_DATA_STEREO3D. Map it onto mp_image_params.stereo3d. Frames coming back from libavfilter keep mpv's own value restored from opaque_ref. Co-authored-by: Kacper Michajłow <kasper93@gmail.com>
fix_image_params() unconditionally overwrote the layout with the container tag, so the value mp_image_from_av_frame() reads from AV_FRAME_DATA_STEREO3D never reached the output. Fall back to the tag only when the bitstream signals nothing, the same rule rotation already follows. Fixes: mpv-player#18489 Co-authored-by: Kacper Michajłow <kasper93@gmail.com>
3bcee81 to
e2e62db
Compare
set_params() treated 0 as unset, but 0 is mono, so stereo-in=mono was a no-op and the only way to drop a stereo 3D mode was stereo-in=no, which wrote the invalid value -1 into the image params. Use -1 as the unset default like rotate does, so no keeps the source mode and mono overrides it.
mpv stopped inserting vf_stereo3d itself in a5610b2, the names only need to stay recognizable next to the stereo3d filter documentation that DOCS/man/vf.rst points to. The filter also accepts icl and icr as input formats.
The last caller went away with the rpi code in 343a5fd. It also regenerated the OSD with stereo mode 0, so any new user would have reset the per eye OSD layout on stereo 3D content. Drop the change tracking state that only existed for it.
5 participants
Fixes #18489.
What this patches:
StereoModeelement. A layout signalled in the video stream, the H.264/HEVCframe_packing_arrangementSEI (payload type 45), or the MP4st3dbox, is ignored, and such files play asstereo-in=mono.libavcodec already decodes that SEI into
AVStereo3Dframe side data and hands it to mpv. mpv never asks for it.Reproduction steps:
Three files isolating each signal, built in three commands using ffmpeg:
Before this patch
sei_only.mp4reportsstereo-in=mono; after it reportsstereo-in=sbs2l.Why the first frame was not enough:
The in-band signal is attached only to the frames that carry it. Measured files.
Results
Verified on Debian 14, x86_64, libavcodec 63.1.101, libplacebo 7.360.1, built from this tree. Real files are ordinary 3D videos, not synthesized for the test.
No container-tagged file changes behaviour, and files with no signal stay
mono.The inverted case is the one that showed why metadata beats guessing: that file's SEI declares
side by side (inverted), the right eye first type, while its filename saysSBS.Before this change mpv showed it flat; a player guessing from the filename would swap the viewer's eyes. Only the declared layout gets it right.
Scope:
This makes mpv detect the layout. It does not change what mpv does with it, no automatic conversion is added, and
--vf=format:stereo-in=...still overrides as before.Files that already worked are unaffected.
Relation to existing issues:
AVStereo3Dbeing wiredthrough this path.
sbsl/ablchoices; its reporter's file is aMatroska whose tag mpv reads correctly. That is the layout vocabulary; this is the
detection source.
Why this is becoming more common:
HandBrake merged support for writing this SEI (PR #8100), and FFmpeg has an open request for a lossless injector for it (#24531), so files carrying the in-band signal are expected to keep appearing.
Testing:
Tested, per
DOCS/contribute.md. Built from this tree and run against the three control files and a 26-file library of real 3D videos. Happy to adjust the mapping's placement — it could live next to mp_stereo3d_names[]incsputils.c` instead of inline — or to split the fallback differently if you would rather the container tag always win.Disclosure:
An AI partner helped produce the outcomes. The words here are mine, edited in the browser.
Every result above was executed and measured on my machine, not inferred;
The patch was compiled, the before/after values come from running both binaries against the same files, and the side-data-per-keyframe counts were measured with
ffmpeg -vf showinfo.