Open dyphire opened 11 months ago
llyyr
commented
11 months ago That commit only affects non-ass formats, this is a "bug" report for libass not mpv.
I've also noticed that ass subs with an end duration longer than the file's duration just get completely ignored.
I'd consider this a feature.
Jules-A
commented
11 months ago I'd consider this a feature.
Crunchyroll does it sometimes and it displays fine in Firefox. I assume it's a lazy way of saying play sub until the end because I'm too lazy to figure out when it ends...
dyphire
commented
11 months ago I've also noticed that ass subs with an end duration longer than the file's duration just get completely ignored.
I think this should be normal behavior. Do other players handle it differently?
That commit only affects non-ass formats, this is a "bug" report for libass not mpv.
I'm not sure if this issue is related to that commit, but I thinks this is a "bug" for mpv not libass because other applications that use libass can handle it correctly, for example: vlc, aegisub.
llyyr
commented
11 months ago I'm not sure if this issue is related to that commit, but I thinks this is a "bug" for mpv not libass because other applications that use libass can handle it correctly, for example: vlc, aegisub.
I'll take a look at what they do, but mpv uses the ass_process_chunk function which explicitly says
In later libass versions (since LIBASS_VERSION==0x01300001), using this function means you agree not to modify events manually, or using other functions manipulating the event list like ass_process_data().
So if they use the same method and do modify events then it's an API violation...
Also what is "correctly" here?
I assume it's a lazy way of saying play sub until the end because I'm too lazy to figure out when it ends...
They have the video, how is it hard to know when the video file ends? Aegisub shows this you to without even needing to click anywhere.
Also >Crunchyroll
Multiple times they've typo'd 240 instead of 24 and published episodes that were 4 hours long on their browser player instead of 24 minutes long.
Subtitles cannot exceed the audio or video length (whichever is more), any video player doing it differently is doing it wrong.
dyphire
commented
11 months ago I'm not sure if this issue is related to that commit, but I thinks this is a "bug" for mpv not libass because other applications that use libass can handle it correctly, for example: vlc, aegisub.
I'll take a look at what they do, but mpv uses the ass_process_chunk function which explicitly says
In later libass versions (since LIBASS_VERSION==0x01300001), using this function means you agree not to modify events manually, or using other functions manipulating the event list like ass_process_data().
So if they use the same method and do modify events then it's an API violation...
Also what is "correctly" here?
I don't want to argue with you about the correct definition, it deviates from the issue and is just an inappropriate expression. mpv currently displays subtitles with abnormal duration until the next subtitle event occurs, which is clearly not what anyone expected. In fact, mpv has recently fixed a similar error for pgs subtitles: https://github.com/mpv-player/mpv/issues/8202, https://github.com/mpv-player/mpv/issues/10051, which is also normal work in vlc.
I agree that the API specification should be followed, but only when it is beneficial. What is the reason not to fix it if there are errors? mpv already has many hackers used to fix various known problems, why is this an exception?
Jules-A
commented
11 months ago Oh wow, actually I figured out the issue, the last event wasn't actually getting embedded by ffmpeg, if I use subs from the actual .ass MPV plays it fine also so not a libass issue (at least for playback) but ffmpeg. Sorry guys :(
llyyr
commented
11 months ago I don't want to argue with you about the correct definition, it deviates from the issue and is just an inappropriate expression.
i'm not arguing, I'm asking how other players handle it so I can use it as a reference when attempting to fix it. Do they not display the sub at all, since it has a negative duration?
dyphire
commented
11 months ago I don't want to argue with you about the correct definition, it deviates from the issue and is just an inappropriate expression.
i'm not arguing, I'm asking how other players handle it so I can use it as a reference when attempting to fix it. Do they not display the sub at all, since it has a negative duration?
You got me:joy:. I don't know how they are handled internally, so I opened an issue to report it.
llyyr
commented
11 months ago You don't need to know how they're handled internally to know how they handle it.
Your original post says
The duration of the sample subtitles is displayed normally, just like vlc.
What is "normally" in this case? The subtitle duration is negative. Can you record vlc playback of this subtitle file as well?
dyphire
commented
11 months ago You don't need to know how they're handled internally to know how they handle it.
Your original post says
The duration of the sample subtitles is displayed normally, just like vlc.
What is "normally" in this case? The subtitle duration is negative. Can you record vlc playback of this subtitle file as well?
This is the subtitle rendering of vlc playback:
https://github.com/mpv-player/mpv/assets/61936050/e10ebb19-d642-4b3c-a94b-fffa6a5d4332
Edit: It is worth mentioning that if I add the following content below the subtitle line of an incorrect duration, the subtitle display behavior of mpv will be consistent with that of vlc.
Dialogue: 1,0:23:57.49,0:23:57.49,ed - jp,,0,0,0,or
Dialogue: 1,0:23:57.50,0:23:57.50,ed - jp,,0,0,0,Edit: Ah, this is actually covered by an empty subtitle rendering event.
llyyr
commented
11 months ago After investigating further, this happens because of this workaround in ffmpeg
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/subtitles.c#L216
This ensures that the duration of subtitles is never negative, if it is then it assumes the duration is (current event start) - (next event start), and if there's no next event then it doesn't branch into that line. This workaround is actually necessary and removing it would break many other older subtitles formats.
The reason this breaks for that file is because there's another empty event after this line:
Dialogue: 0,0:24:26.82,0:24:28.82,Default,,0,0,0,,This causes ffmpeg to set the duration to 24:26.82 - 23:57.49.
You could make the workaround also check if the next event has any dialogue or not, but I don't see any point on entertaining such severely broken files. As far as I'm concerned, this is a no-fix. If anyone else wants to have a go at fixing this in ffmpeg, feel free.
The workaround is actually necessary and a net positive in every situation except severely broken files like this one. You're free to delete the final line the the file instead, then the file will be rendered correctly, despite the negative duration.
dyphire
commented
11 months ago After investigating further, this happens because of this workaround in ffmpeg
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/subtitles.c#L216
This ensures that the duration of subtitles is never negative, if it is then it assumes the duration is
(current event start) - (next event start), and if there's no next event then it doesn't branch into that line. This workaround is actually necessary and removing it would break many other older subtitles formats.The reason this breaks for that file is because there's another empty event after this line:
Dialogue: 0,0:24:26.82,0:24:28.82,Default,,0,0,0,,This causes ffmpeg to set the duration to
24:26.82 - 23:57.49.You could make the workaround also check if the next event has any dialogue or not, but I don't see any point on entertaining such severely broken files. As far as I'm concerned, this is a no-fix. If anyone else wants to have a go at fixing this in ffmpeg, feel free.
The workaround is actually necessary and a net positive in every situation except severely broken files like this one. You're free to delete the final line the the file instead, then the file will be rendered correctly, despite the negative duration.
In fact, the sample file is not the original subtitle. To make the problem more obvious, I removed the following subtitles. This is the original subtitle file: [Nekomoe kissaten&VCB-Studio] Mahou Shoujo Site [01][Ma10p_1080p][x265_flac].sc.zip
The error in the original subtitle file is not as serious, but there are still problems displaying it in mpv.
https://github.com/mpv-player/mpv/assets/61936050/89832a68-f42a-4ee5-892a-737646645a98
The workaround is actually necessary and a net positive in every situation except severely broken files like this one. You're free to delete the final line the the file instead, then the file will be rendered correctly, despite the negative duration.
This does not apply to the original subtitles. I don't understand why you think ffmpeg's behavior is not a problem, even if other applications (vlc, aegisub, mpc-hc) haven't adopted this handling and appear smarter (without these error).
I can understand that you don't want to fix it. I want to keep it open and wait for someone with other ideas about it.
dyphire
commented
11 months ago I also don't think we should touch the code in ffmpeg, maybe we can handle it in mpv.
Process in the find_timestamp function of sub/sd_ass.c, just as it has already been done in the get_current function of sub/sd_lavc.c.
llyyr
commented
11 months ago mpv never knows the actual "end" pts of an ass event, only for non-ass subs we convert via lavc conv.
If you look at this define
#define END(ev) ((ev)->Start + (ev)->Duration)
We calculate the end by adding the start and duration, both values we get from ffmpeg. The duration value is already incorrect here (it's 24:26.82 - 23:57.49), so I don't think there's no way of fixing this in mpv short of implementing our own ass demuxer. It's definitely possible that I'm wrong though, so if you think there's a way then feel free to submit a patch.
dyphire
commented
11 months ago mpv never knows the actual "end" pts of an ass event, only for non-ass subs we convert via lavc conv.
If you look at this define
#define END(ev) ((ev)->Start + (ev)->Duration)We calculate the end by adding the start and duration, both values we get from ffmpeg. The duration value is already incorrect here (it's
24:26.82 - 23:57.49), so I don't think there's no way of fixing this in mpv short of implementing our own ass demuxer. It's definitely possible that I'm wrong though, so if you think there's a way then feel free to submit a patch.
Yes, you are right. Through studying the code, I found that mpv did not obtain the original timestamp of the subtitle end time, so it is impossible to fix it only in mpv.
llyyr
commented
11 months ago even if other applications (vlc, aegisub, mpc-hc) haven't adopted this handling and appear smarter (without these error).
This heavily depends on the specific subtitles used. Although I suppose I could look into only applying that workaround only for non-ASS subs in ffmpeg, since only old subtitle formats need that handling most likely.
llyyr
commented
11 months ago Never mind actually! I grepped through all the anime I have and there are 3 different files I have that do have negative durations on dialogue events which aren't shown at all in VLC but are shown in mpv. This special handling is an improvement the vast majority of time, even for ASS subs. This is a massive edge case where it isn't.
The duration of the sample subtitles is displayed normally, just like vlc.
Fun fact, VLC isn't displaying is normally either. The effect is skipped, it just looks normal because it's hard to tell the difference between karaoke animation.
However, if you want to then you can apply this patch for your local ffmpeg
diff --git a/libavformat/assdec.c b/libavformat/assdec.c
index bf7b8a73a2..6b37af15e8 100644
--- a/libavformat/assdec.c
+++ b/libavformat/assdec.c
@@ -123,6 +123,7 @@ static int ass_read_header(AVFormatContext *s)
av_bprint_init(&rline, 0, AV_BPRINT_SIZE_UNLIMITED);
ass->q.keep_duplicates = 1;
+ ass->q.keep_negative_dur = 1;
for (;;) {
int64_t pos = get_line(&line, &tr);
diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
index 3413763c7b..899dcc4943 100644
--- a/libavformat/subtitles.c
+++ b/libavformat/subtitles.c
@@ -211,9 +211,14 @@ void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
qsort(q->subs, q->nb_subs, sizeof(*q->subs),
q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
: cmp_pkt_sub_pos_ts);
- for (i = 0; i < q->nb_subs; i++)
- if (q->subs[i]->duration < 0 && i < q->nb_subs - 1 && q->subs[i + 1]->pts - (uint64_t)q->subs[i]->pts <= INT64_MAX)
- q->subs[i]->duration = q->subs[i + 1]->pts - q->subs[i]->pts;
+
+ if (!q->keep_negative_dur) {
+ for (i = 0; i < q->nb_subs; i++)
+ if (q->subs[i]->duration < 0 &&
+ i < q->nb_subs - 1 &&
+ q->subs[i + 1]->pts - (uint64_t)q->subs[i]->pts <= INT64_MAX)
+ q->subs[i]->duration = q->subs[i + 1]->pts - q->subs[i]->pts;
+ }
if (!q->keep_duplicates)
drop_dups(log_ctx, q);
diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h
index f4993fe20d..84a576aa6a 100644
--- a/libavformat/subtitles.h
+++ b/libavformat/subtitles.h
@@ -101,12 +101,13 @@ int ff_text_peek_r8(FFTextReader *r);
void ff_text_read(FFTextReader *r, char *buf, size_t size);
typedef struct {
- AVPacket **subs; ///< array of subtitles packets
+ AVPacket **subs; ///< array of subtitles packets
int nb_subs; ///< number of subtitles packets
int allocated_size; ///< allocated size for subs
int current_sub_idx; ///< current position for the read packet callback
enum sub_sort sort; ///< sort method to use when finalizing subtitles
int keep_duplicates; ///< set to 1 to keep duplicated subtitle events
+ int keep_negative_dur; ///< set to 1 to keep negative event duration
} FFDemuxSubtitlesQueue;
/**Never mind actually! I grepped through all the anime I have and there are 3 different files I have that do have negative durations on dialogue events which aren't shown at all in VLC but are shown in mpv. This special handling is an improvement the vast majority of time, even for ASS subs. This is a massive edge case where it isn't.
The duration of the sample subtitles is displayed normally, just like vlc.
Fun fact, VLC isn't displaying is normally either. The effect is skipped, it just looks normal because it's hard to tell the difference between karaoke animation.
In fact, there are very few erroneous ass subtitles with similar negative durations. I never noticed the existence of such a problem until someone reported it.
However, if you want to then you can apply this patch for your local ffmpeg
I personally prefer the behavior of only handling non-ASS subs just like your patches, but considering the marginalization of the scene, even maintaining the status quo is acceptable.
Edit: In addition, I believe that there are still areas for optimization in the existing processing of ffmpeg, and a duration threshold should be set for the recalculated timestamps to avoid negative timestamped subtitles staying on the screen for a long time. What do you think? @llyyr
Jules-A
commented
11 months ago I believe that there are still areas for optimization in the existing processing of ffmpeg
What does -fix_sub_duration do for this particular sub? When the sub's end time is past the video's duration it just deletes the event (even when it's the last event)... It would be nice if it capped it to the end of the video (to match browsers and MPV behavior) instead but again that's not a MPV issue.
Important Information
Provide following Information:
Reproduction steps
Expected behavior
The duration of the sample subtitles is displayed normally, just like vlc.
Actual behavior
The duration of the last subtitle event is incorrect, it always appears on the screen until the end of the video.
https://github.com/mpv-player/mpv/assets/61936050/950ea221-2ec0-4934-8fd4-f6ae222923cc
The problem here is that the last subtitle event in the sample subtitle file has an incorrect duration, and mpv did not process it correctly.
Commit https://github.com/mpv-player/mpv/commit/740b7013ba827ce5a9d48138af5bd2e8f5d54710 introduced handling for subtitles with unknown duration, but it is obvious that this processing method is not correct enough and needs to be fixed.Maybe we can refer to how it was handled insd_lavcLog file
mpv.log
Sample file
[Nekomoe kissaten&VCB-Studio] Mahou Shoujo Site [01][Ma10p_1080p][x265_flac].sc.zip