Update the way timestamp is handled (#1153)

1. When comparing the timestamp and PTS, convert them into AVRational
instead of double, so that the comparison is more accurate.
2. Use the same timestamp filtering mechanism in Packets and decoding.
3. Do not use the `trim` filter for timestamp mechanism. Instead filter
manually, using the above mechanism.

The new implementation gives a result closer to Python's slice notation.
For example in one of the test, we used `timestamp=(2.88, 2.89)`. The
`trim` filter will discard the frame at 2.88, while the new
implementation keeps it.

Author: mthrok

src/libspdl/core/detail/ffmpeg/decoder.cpp

@@ -10,6 +10,7 @@
 
 #include "libspdl/core/detail/ffmpeg/ctx_utils.h"
 #include "libspdl/core/detail/ffmpeg/logging.h"
+#include "libspdl/core/detail/ffmpeg/rational_utils.h"
 #include "libspdl/core/detail/tracing.h"
 
 #include <glog/logging.h>
@@ -142,6 +143,10 @@ Rational DecoderImpl<media>::get_output_time_base() const {
   return {codec_ctx->time_base.num, codec_ctx->time_base.den};
 }
 
+// For audio and image.
+// Note: when decoding audio with timestamp, we rely on `atrim` filter
+// for handling timestamp.
+// This is handled through high-level Python interface.
 template <MediaType media>
 FramesPtr<media> DecoderImpl<media>::decode_and_flush(
     PacketsPtr<media> packets,
@@ -161,6 +166,55 @@ FramesPtr<media> DecoderImpl<media>::decode_and_flush(
   return ret;
 }
 
+// Specialization for video.
+// For video we want to ensure the half-open range.
+// Originally we used `trim` filter like how audio is processed above,
+// but this was not properly handling the half-open range, so we have
+// specialization for video.
+template <>
+VideoFramesPtr DecoderImpl<MediaType::Video>::decode_and_flush(
+    VideoPacketsPtr packets,
+    int num_frames) {
+  auto time_base = get_output_time_base();
+  auto [has_timestamp, start, end] =
+      [&]() -> std::tuple<bool, AVRational, AVRational> {
+    if (packets->timestamp) {
+      auto [start_time, end_time] = *packets->timestamp;
+      return std::make_tuple(
+          true,
+          av_d2q(start_time, AV_TIME_BASE),
+          av_d2q(end_time, AV_TIME_BASE));
+    }
+    return std::make_tuple(false, AVRational{-1, 0}, AVRational{1, 0});
+  }();
+
+  auto ret = std::make_unique<VideoFrames>(packets->id, get_output_time_base());
+  auto gen = decode_packets(
+      codec_ctx, packets->pkts.get_packets(), filter_graph, true);
+  int num_yielded = 0;
+  while (gen) {
+    // For video, we manualy apply timestamps.
+    auto frame = gen().release();
+    if (has_timestamp && frame) {
+      auto frame_pts = detail::to_rational(frame->pts, time_base);
+      if (!is_within_window(frame_pts, start, end)) {
+        av_frame_free(&frame);
+        continue;
+      }
+    }
+    ret->push_back(frame);
+    num_yielded += 1;
+    if (num_frames > 0 && num_yielded >= num_frames) {
+      break;
+    }
+  }
+  return ret;
+}
+
+// For audio and image.
+// Note: when decoding audio with timestamp, we rely on `atrim` filter
+// for handling timestamp.
+// This is handled through high-level Python interface.
 template <MediaType media>
 FramesPtr<media> DecoderImpl<media>::decode(PacketsPtr<media> packets) {
   auto ret =
@@ -173,6 +227,47 @@ FramesPtr<media> DecoderImpl<media>::decode(PacketsPtr<media> packets) {
   return ret;
 }
 
+// Specialization for video.
+// For video we want to ensure the half-open range.
+// Originally we used `trim` filter like how audio is processed above,
+// but this was not properly handling the half-open range, so we have
+// specialization for video.
+template <>
+VideoFramesPtr DecoderImpl<MediaType::Video>::decode(VideoPacketsPtr packets) {
+  auto ret = std::make_unique<VideoFrames>(packets->id, get_output_time_base());
+  auto gen = decode_packets(
+      codec_ctx, packets->pkts.get_packets(), filter_graph, false);
+
+  auto time_base = get_output_time_base();
+  AVRational start = {0, 1};
+  AVRational end = {0, 1};
+  bool has_timestamp = false;
+
+  if (packets->timestamp) {
+    auto [start_time, end_time] = *packets->timestamp;
+    start = av_d2q(start_time, AV_TIME_BASE);
+    end = av_d2q(end_time, AV_TIME_BASE);
+    has_timestamp = true;
+  }
+
+  while (gen) {
+    auto frame = gen().release();
+
+    if (has_timestamp && frame) {
+      auto frame_pts = AVRational{
+          static_cast<int>(frame->pts * time_base.num), time_base.den};
+
+      if (!is_within_window(frame_pts, start, end)) {
+        av_frame_free(&frame);
+        continue;
+      }
+    }
+
+    ret->push_back(frame);
+  }
+  return ret;
+}
+
 template <MediaType media>
 FramesPtr<media> DecoderImpl<media>::flush() {
   auto ret = std::make_unique<Frames<media>>(

src/libspdl/core/packets.cpp

@@ -247,7 +247,8 @@ extract_packets(const VideoPacketsPtr& src, size_t start, size_t end) {
   auto ret = std::make_unique<VideoPackets>();
   ret->src = src->src;
   ret->codec = src->codec;
-  ret->timestamp = src->timestamp;
+  // Do not preserve timestamp as indices are already adjusted
+  ret->timestamp = std::nullopt;
   for (size_t t = start; t < end; ++t) {
     ret->pkts.push(CHECK_AVALLOCATE(av_packet_clone(src_packets[t])));
   }

src/spdl/io/_preprocessing.py

@@ -254,9 +254,12 @@ def get_filter_desc(
     """
     match type(packets):
         case _libspdl.AudioPackets:
+            # When audio packets have `timestamp` attribute, we delegate to `atrim` filter.
             return get_audio_filter_desc(timestamp=packets.timestamp, **filter_args)
         case _libspdl.VideoPackets:
-            return get_video_filter_desc(timestamp=packets.timestamp, **filter_args)
+            # When video packets have `timestamp` attribute, we manually filter the frame
+            # in the decoding logic, so we don't use `trim` filter.
+            return get_video_filter_desc(timestamp=None, **filter_args)
         case _libspdl.ImagePackets:
             return get_video_filter_desc(timestamp=None, **filter_args)
         case _: