Fix: VSE indexer seeking not working correctly

Because of the added sanity checks in rB14508ef100c9 (D11492), seeking
in proxies would not work correctly any more. This is because it wasn't
working as intended before, but in most cases this wouldn't be
noticeable. However now when the sanity checks are tripped it is very
noticeable that something is wrong

The indexer tried to use dts values for time stamps when we used pts in
our decode functions to get the time positions. This would make it
start in the wrong GOP frames when searching. Now that we enforce no
crossing of GOP frames when decoding after seek, this would lead to
issues.

Now we correctly use pts (or dts if pts is not available) and thus we
don't have any seeking issues because of time stamp format missmatch.

Reviewed By: Richard Antalik

Differential Revision: http://developer.blender.org/D11561

3 files changed, 85 insertions, 32 deletions

diff --git a/source/blender/imbuf/intern/IMB_indexer.h b/source/blender/imbuf/intern/IMB_indexer.h
index 363ad45e0e6..37309ccc13a 100644
--- a/source/blender/imbuf/intern/IMB_indexer.h
+++ b/source/blender/imbuf/intern/IMB_indexer.h
@@ -49,6 +49,7 @@
 typedef struct anim_index_entry {
   int frameno;
   uint64_t seek_pos;
+  uint64_t seek_pos_pts;
   uint64_t seek_pos_dts;
   uint64_t pts;
 } anim_index_entry;
@@ -77,14 +78,19 @@ typedef struct anim_index_builder {
 } anim_index_builder;
 
 anim_index_builder *IMB_index_builder_create(const char *name);
-void IMB_index_builder_add_entry(
-    anim_index_builder *fp, int frameno, uint64_t seek_pos, uint64_t seek_pos_dts, uint64_t pts);
+void IMB_index_builder_add_entry(anim_index_builder *fp,
+                                 int frameno,
+                                 uint64_t seek_pos,
+                                 uint64_t seek_pos_pts,
+                                 uint64_t seek_pos_dts,
+                                 uint64_t pts);
 
 void IMB_index_builder_proc_frame(anim_index_builder *fp,
                                   unsigned char *buffer,
                                   int data_size,
                                   int frameno,
                                   uint64_t seek_pos,
+                                  uint64_t seek_pos_pts,
                                   uint64_t seek_pos_dts,
                                   uint64_t pts);
 
@@ -92,6 +98,7 @@ void IMB_index_builder_finish(anim_index_builder *fp, int rollback);
 
 struct anim_index *IMB_indexer_open(const char *name);
 uint64_t IMB_indexer_get_seek_pos(struct anim_index *idx, int frame_index);
+uint64_t IMB_indexer_get_seek_pos_pts(struct anim_index *idx, int frame_index);
 uint64_t IMB_indexer_get_seek_pos_dts(struct anim_index *idx, int frame_index);
 
 int IMB_indexer_get_frame_index(struct anim_index *idx, int frameno);
diff --git a/source/blender/imbuf/intern/anim_movie.c b/source/blender/imbuf/intern/anim_movie.c
index b65c3e364db..739ec988121 100644
--- a/source/blender/imbuf/intern/anim_movie.c
+++ b/source/blender/imbuf/intern/anim_movie.c
@@ -924,7 +924,7 @@ static int ffmpeg_decode_video_frame(struct anim *anim)
       anim->pFrameComplete = avcodec_receive_frame(anim->pCodecCtx, anim->pFrame) == 0;
 
       if (anim->pFrameComplete) {
-        anim->cur_pts = av_get_pts_from_frame(anim->pFormatCtx, anim->pFrame);
+        anim->cur_pts = av_get_pts_from_frame(anim->pFrame);
 
         if (anim->pFrame->key_frame) {
           anim->cur_key_frame_pts = anim->cur_pts;
@@ -949,7 +949,7 @@ static int ffmpeg_decode_video_frame(struct anim *anim)
     anim->pFrameComplete = avcodec_receive_frame(anim->pCodecCtx, anim->pFrame) == 0;
 
     if (anim->pFrameComplete) {
-      anim->cur_pts = av_get_pts_from_frame(anim->pFormatCtx, anim->pFrame);
+      anim->cur_pts = av_get_pts_from_frame(anim->pFrame);
 
       if (anim->pFrame->key_frame) {
         anim->cur_key_frame_pts = anim->cur_pts;
@@ -1164,6 +1164,7 @@ static int ffmpeg_generic_seek_workaround(struct anim *anim, int64_t *requested_
       if (anim->cur_packet->stream_index == anim->videoStream) {
         break;
       }
+      av_packet_unref(read_packet);
     }
 
     /* If this packet contains I-frame, exit loop. This should be the frame that we need. */
@@ -1197,14 +1198,17 @@ static int ffmpeg_seek_to_key_frame(struct anim *anim, int position, struct anim
       /* No need to seek, return early. */
       return 0;
     }
+    uint64_t pts;
     uint64_t dts;
 
     pos = IMB_indexer_get_seek_pos(tc_index, new_frame_index);
+    pts = IMB_indexer_get_seek_pos_pts(tc_index, new_frame_index);
     dts = IMB_indexer_get_seek_pos_dts(tc_index, new_frame_index);
 
-    anim->cur_key_frame_pts = pos;
+    anim->cur_key_frame_pts = timestamp_from_pts_or_dts(pts, dts);
 
     av_log(anim->pFormatCtx, AV_LOG_DEBUG, "TC INDEX seek pos = %" PRId64 "\n", pos);
+    av_log(anim->pFormatCtx, AV_LOG_DEBUG, "TC INDEX seek pts = %" PRIu64 "\n", pts);
     av_log(anim->pFormatCtx, AV_LOG_DEBUG, "TC INDEX seek dts = %" PRIu64 "\n", dts);
 
     if (ffmpeg_seek_by_byte(anim->pFormatCtx)) {
@@ -1214,8 +1218,9 @@ static int ffmpeg_seek_to_key_frame(struct anim *anim, int position, struct anim
       av_update_cur_dts(anim->pFormatCtx, v_st, dts);
     }
     else {
-      av_log(anim->pFormatCtx, AV_LOG_DEBUG, "... using DTS pos\n");
-      ret = av_seek_frame(anim->pFormatCtx, anim->videoStream, dts, AVSEEK_FLAG_BACKWARD);
+      av_log(anim->pFormatCtx, AV_LOG_DEBUG, "... using PTS pos\n");
+      ret = av_seek_frame(
+          anim->pFormatCtx, anim->videoStream, anim->cur_key_frame_pts, AVSEEK_FLAG_BACKWARD);
     }
   }
   else {
@@ -1243,24 +1248,29 @@ static int ffmpeg_seek_to_key_frame(struct anim *anim, int position, struct anim
         }
         av_packet_unref(current_gop_start_packet);
       }
-      bool same_gop = current_gop_start_packet->pts == anim->cur_key_frame_pts;
+      int64_t gop_pts = timestamp_from_pts_or_dts(current_gop_start_packet->pts,
+                                                  current_gop_start_packet->dts);
+
+      av_packet_free(&current_gop_start_packet);
+      bool same_gop = gop_pts == anim->cur_key_frame_pts;
 
       if (same_gop && position > anim->cur_position) {
         /* Change back to our old frame position so we can simply continue decoding from there. */
         AVPacket *temp = av_packet_alloc();
         while (av_read_frame(anim->pFormatCtx, temp) >= 0) {
-          if (temp->stream_index == anim->videoStream && temp->pts == anim->cur_packet->pts) {
+          int64_t temp_pts = timestamp_from_pts_or_dts(temp->pts, temp->dts);
+          int64_t cur_pts = timestamp_from_pts_or_dts(anim->cur_packet->pts,
+                                                      anim->cur_packet->dts);
+          if (temp->stream_index == anim->videoStream && temp_pts == cur_pts) {
             break;
           }
           av_packet_unref(temp);
         }
-        av_packet_free(&current_gop_start_packet);
         av_packet_free(&temp);
         return 0;
       }
 
-      anim->cur_key_frame_pts = current_gop_start_packet->pts;
-      av_packet_free(&current_gop_start_packet);
+      anim->cur_key_frame_pts = gop_pts;
       /* Seek back so we are at the correct position after we decoded a frame. */
       av_seek_frame(anim->pFormatCtx, -1, pos, AVSEEK_FLAG_BACKWARD);
     }
@@ -1278,6 +1288,8 @@ static int ffmpeg_seek_to_key_frame(struct anim *anim, int position, struct anim
            pts_to_search,
            ret);
   }
+  /* Flush the internal buffers of ffmpeg. This needs to be done after seeking to avoid decoding
+   * errors. */
   avcodec_flush_buffers(anim->pCodecCtx);
 
   anim->cur_pts = -1;
diff --git a/source/blender/imbuf/intern/indexer.c b/source/blender/imbuf/intern/indexer.c
index a8733da39a7..a530acb15b0 100644
--- a/source/blender/imbuf/intern/indexer.c
+++ b/source/blender/imbuf/intern/indexer.c
@@ -50,7 +50,7 @@
 #  include <libavutil/imgutils.h>
 #endif
 
-static const char magic[] = "BlenMIdx";
+static const char binary_header_str[] = "BlenMIdx";
 static const char temp_ext[] = "_part";
 
 static const int proxy_sizes[] = {IMB_PROXY_25, IMB_PROXY_50, IMB_PROXY_75, IMB_PROXY_100};
@@ -65,7 +65,7 @@ static int tc_types[] = {
 };
 #endif
 
-#define INDEX_FILE_VERSION 1
+#define INDEX_FILE_VERSION 2
 
 /* ----------------------------------------------------------------------
  * - time code index functions
@@ -96,16 +96,25 @@ anim_index_builder *IMB_index_builder_create(const char *name)
     return NULL;
   }
 
-  fprintf(rv->fp, "%s%c%.3d", magic, (ENDIAN_ORDER == B_ENDIAN) ? 'V' : 'v', INDEX_FILE_VERSION);
+  fprintf(rv->fp,
+          "%s%c%.3d",
+          binary_header_str,
+          (ENDIAN_ORDER == B_ENDIAN) ? 'V' : 'v',
+          INDEX_FILE_VERSION);
 
   return rv;
 }
 
-void IMB_index_builder_add_entry(
-    anim_index_builder *fp, int frameno, uint64_t seek_pos, uint64_t seek_pos_dts, uint64_t pts)
+void IMB_index_builder_add_entry(anim_index_builder *fp,
+                                 int frameno,
+                                 uint64_t seek_pos,
+                                 uint64_t seek_pos_pts,
+                                 uint64_t seek_pos_dts,
+                                 uint64_t pts)
 {
   fwrite(&frameno, sizeof(int), 1, fp->fp);
   fwrite(&seek_pos, sizeof(uint64_t), 1, fp->fp);
+  fwrite(&seek_pos_pts, sizeof(uint64_t), 1, fp->fp);
   fwrite(&seek_pos_dts, sizeof(uint64_t), 1, fp->fp);
   fwrite(&pts, sizeof(uint64_t), 1, fp->fp);
 }
@@ -115,6 +124,7 @@ void IMB_index_builder_proc_frame(anim_index_builder *fp,
                                   int data_size,
                                   int frameno,
                                   uint64_t seek_pos,
+                                  uint64_t seek_pos_pts,
                                   uint64_t seek_pos_dts,
                                   uint64_t pts)
 {
@@ -122,13 +132,14 @@ void IMB_index_builder_proc_frame(anim_index_builder *fp,
     anim_index_entry e;
     e.frameno = frameno;
     e.seek_pos = seek_pos;
+    e.seek_pos_pts = seek_pos_pts;
     e.seek_pos_dts = seek_pos_dts;
     e.pts = pts;
 
     fp->proc_frame(fp, buffer, data_size, &e);
   }
   else {
-    IMB_index_builder_add_entry(fp, frameno, seek_pos, seek_pos_dts, pts);
+    IMB_index_builder_add_entry(fp, frameno, seek_pos, seek_pos_pts, seek_pos_dts, pts);
   }
 }
 
@@ -159,22 +170,26 @@ struct anim_index *IMB_indexer_open(const char *name)
   int i;
 
   if (!fp) {
+    fprintf(stderr, "Couldn't open indexer file: %s\n", name);
     return NULL;
   }
 
   if (fread(header, 12, 1, fp) != 1) {
+    fprintf(stderr, "Couldn't read indexer file: %s\n", name);
     fclose(fp);
     return NULL;
   }
 
   header[12] = 0;
 
-  if (memcmp(header, magic, 8) != 0) {
+  if (memcmp(header, binary_header_str, 8) != 0) {
+    fprintf(stderr, "Error reading %s: Binary file type string missmatch\n", name);
     fclose(fp);
     return NULL;
   }
 
   if (atoi(header + 9) != INDEX_FILE_VERSION) {
+    fprintf(stderr, "Error reading %s: File version missmatch\n", name);
     fclose(fp);
     return NULL;
   }
@@ -187,6 +202,7 @@ struct anim_index *IMB_indexer_open(const char *name)
 
   idx->num_entries = (ftell(fp) - 12) / (sizeof(int) +      /* framepos */
                                          sizeof(uint64_t) + /* seek_pos */
+                                         sizeof(uint64_t) + /* seek_pos_pts */
                                          sizeof(uint64_t) + /* seek_pos_dts */
                                          sizeof(uint64_t)   /* pts */
                                         );
@@ -200,12 +216,13 @@ struct anim_index *IMB_indexer_open(const char *name)
   for (i = 0; i < idx->num_entries; i++) {
     items_read += fread(&idx->entries[i].frameno, sizeof(int), 1, fp);
     items_read += fread(&idx->entries[i].seek_pos, sizeof(uint64_t), 1, fp);
+    items_read += fread(&idx->entries[i].seek_pos_pts, sizeof(uint64_t), 1, fp);
     items_read += fread(&idx->entries[i].seek_pos_dts, sizeof(uint64_t), 1, fp);
     items_read += fread(&idx->entries[i].pts, sizeof(uint64_t), 1, fp);
   }
 
-  if (UNLIKELY(items_read != idx->num_entries * 4)) {
-    perror("error reading animation index file");
+  if (UNLIKELY(items_read != idx->num_entries * 5)) {
+    fprintf(stderr, "Error: Element data size missmatch in: %s\n", name);
     MEM_freeN(idx->entries);
     MEM_freeN(idx);
     fclose(fp);
@@ -216,6 +233,7 @@ struct anim_index *IMB_indexer_open(const char *name)
     for (i = 0; i < idx->num_entries; i++) {
       BLI_endian_switch_int32(&idx->entries[i].frameno);
       BLI_endian_switch_uint64(&idx->entries[i].seek_pos);
+      BLI_endian_switch_uint64(&idx->entries[i].seek_pos_pts);
       BLI_endian_switch_uint64(&idx->entries[i].seek_pos_dts);
       BLI_endian_switch_uint64(&idx->entries[i].pts);
     }
@@ -237,6 +255,17 @@ uint64_t IMB_indexer_get_seek_pos(struct anim_index *idx, int frame_index)
   return idx->entries[frame_index].seek_pos;
 }
 
+uint64_t IMB_indexer_get_seek_pos_pts(struct anim_index *idx, int frame_index)
+{
+  if (frame_index < 0) {
+    frame_index = 0;
+  }
+  if (frame_index >= idx->num_entries) {
+    frame_index = idx->num_entries - 1;
+  }
+  return idx->entries[frame_index].seek_pos_pts;
+}
+
 uint64_t IMB_indexer_get_seek_pos_dts(struct anim_index *idx, int frame_index)
 {
   if (frame_index < 0) {
@@ -776,9 +805,10 @@ typedef struct FFmpegIndexBuilderContext {
   IMB_Proxy_Size proxy_sizes_in_use;
 
   uint64_t seek_pos;
-  uint64_t last_seek_pos;
-  uint64_t seek_pos_dts;
   uint64_t seek_pos_pts;
+  uint64_t seek_pos_dts;
+  uint64_t last_seek_pos;
+  uint64_t last_seek_pos_pts;
   uint64_t last_seek_pos_dts;
   uint64_t start_pts;
   double frame_rate;
@@ -931,8 +961,9 @@ static void index_rebuild_ffmpeg_proc_decoded_frame(FFmpegIndexBuilderContext *c
 {
   int i;
   uint64_t s_pos = context->seek_pos;
+  uint64_t s_pts = context->seek_pos_pts;
   uint64_t s_dts = context->seek_pos_dts;
-  uint64_t pts = av_get_pts_from_frame(context->iFormatCtx, in_frame);
+  uint64_t pts = av_get_pts_from_frame(in_frame);
 
   for (i = 0; i < context->num_proxy_sizes; i++) {
     add_to_proxy_output_ffmpeg(context->proxy_ctx[i], in_frame);
@@ -946,15 +977,15 @@ static void index_rebuild_ffmpeg_proc_decoded_frame(FFmpegIndexBuilderContext *c
   context->frameno = floor(
       (pts - context->start_pts) * context->pts_time_base * context->frame_rate + 0.5);
 
-  /* decoding starts *always* on I-Frames,
-   * so: P-Frames won't work, even if all the
-   * information is in place, when we seek
-   * to the I-Frame presented *after* the P-Frame,
-   * but located before the P-Frame within
-   * the stream */
+  int64_t seek_pos_pts = timestamp_from_pts_or_dts(context->seek_pos_pts, context->seek_pos_dts);
 
-  if (pts < context->seek_pos_pts) {
+  if (pts < seek_pos_pts) {
+    /* Decoding starts *always* on I-Frames. In this case our position is
+     * before our seek I-Frame. So we need to pick the previous available
+     * I-Frame to be able to decode this one properly.
+     */
     s_pos = context->last_seek_pos;
+    s_pts = context->last_seek_pos_pts;
     s_dts = context->last_seek_pos_dts;
   }
 
@@ -971,6 +1002,7 @@ static void index_rebuild_ffmpeg_proc_decoded_frame(FFmpegIndexBuilderContext *c
                                    curr_packet->size,
                                    tc_frameno,
                                    s_pos,
+                                   s_pts,
                                    s_dts,
                                    pts);
     }
@@ -1009,10 +1041,12 @@ static int index_rebuild_ffmpeg(FFmpegIndexBuilderContext *context,
     if (next_packet->stream_index == context->videoStream) {
       if (next_packet->flags & AV_PKT_FLAG_KEY) {
         context->last_seek_pos = context->seek_pos;
+        context->last_seek_pos_pts = context->seek_pos_pts;
         context->last_seek_pos_dts = context->seek_pos_dts;
+
         context->seek_pos = next_packet->pos;
-        context->seek_pos_dts = next_packet->dts;
         context->seek_pos_pts = next_packet->pts;
+        context->seek_pos_dts = next_packet->dts;
       }
 
       int ret = avcodec_send_packet(context->iCodecCtx, next_packet);