FFMS / ffms2

An FFmpeg based source library and Avisynth/VapourSynth plugin for easy frame accurate access
Other
582 stars 105 forks source link

Make sure we use the input timebase while generating the hashmap #332

Closed lotharkript closed 5 years ago

lotharkript commented 5 years ago

Without this change, ffmpeg will choose a more appropriate timebase for the output. And in this case, the PTS/DTS in the genfile will be based on the new timebase which will not match the PTS we extract from the indexer.

dwbuiten commented 5 years ago

This is odd, it's causing all the frame durations to be set to 0, if used. Is this expected, or something I should fix in ffmpeg.c?

lotharkript commented 5 years ago

my guess it is something in ffmpeg.

because we are using the input timebased, maybe ffmpeg does not know how to compute duration for those frames.

dwbuiten commented 5 years ago

The following patch seems to be the more correct fix:

commit b1593eaf4b649e43cc43184fab431b21f954263b
Author: Derek Buitenhuis <derek.buitenhuis@gmail.com>
Date:   Fri Oct 12 12:19:05 2018 -0400

    gendata: Get timestamps and frame durations from ffprobe

    We don't want to involve FFmpeg, it's sync code, or any encoder
    in the genration of these values.

    Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>

diff --git a/test/tools/gendata b/test/tools/gendata
index 0c25339..5baef6c 100755
--- a/test/tools/gendata
+++ b/test/tools/gendata
@@ -49,29 +49,31 @@ for (`ffmpeg -i $filename -map $track:0 -f framehash - 2>/dev/null`) {
     chomp;
     s/\s//g;
     my @fields = split(',');
-    push(@timestamps, int($fields[1]) * $ticks);
-    push(@durations, int($fields[3]) * $ticks);
     push(@hashes, $fields[5]);
     $frames++;
 }

 for (`ffprobe -show_frames -select_streams $track $filename 2>/dev/null`) {
+    chomp;
+
     if (/^width/) {
-        chomp;
         my @w = split('=');
         push(@widths, int($w[1]));
     } elsif (/^height/) {
-        chomp;
         my @h = split('=');
         push(@heights, int($h[1]));
     } elsif (/^pix_fmt/) {
-        chomp;
         my @p = split('=');
         push(@pixfmts, $p[1]);
     } elsif (/^key_frame/) {
-        chomp;
         my @k = split('=');
         push(@keyframes, (int($k[1]) == 1 ? "true" : "false"));
+    } elsif (/^pkt_pts=/) {
+        my @t = split('=');
+        push(@timestamps, int($t[1]));
+    } elsif (/^pkt_duration=/) {
+        my @d = split('=');
+        push(@durations, int($d[1]));
     } else {
         next;
     }
lotharkript commented 5 years ago

the patch works. However, if i remember, we could have frames outputted by ffprobe having the same PTS in which in this case, ffmpeg may drop the frame ore recompute the PTS while outputting the hash.

But in ffms2, it will sort the frame by PTS and in the case of 2 frames with the same PTS, the order of them in the index is not guaranty to be the same output as the hash output.

This patch is still better for now than the one i wrote as it does represent more the way the indexer is working.

dwbuiten commented 5 years ago

But in ffms2, it will sort the frame by PTS and in the case of 2 frames with the same PTS, the order of them in the index is not guaranty to be the same output as the hash output.

True when they are fight after eachother, yes. The gendata script was just meant to be a starting point for generating test data, but not definitve, in any case...

As a side note, it would be pretty trivial to write a tool to use the ffindex itself. I just haven't had time...

dwbuiten commented 5 years ago

Superceded by https://github.com/FFMS/ffms2/commit/b1593eaf4b649e43cc43184fab431b21f954263b.