correct `fps` parsing

[Nunatic02]

Motivation

Tis PR is to solve the issue mentioned in https://github.com/imageio/imageio/issues/949

Goal of this PR

Trying to set meta["fps"] to be the fps value parsed from ffmpeg banner of the input video stream, instead of the tbr value of the output video stream. Code Reference

Current Status

Currently, we are setting meta["fps"] to prefer the tbr value of the output video source. For example, for the ffmpeg banner below:

ffmpeg version 5.1.2 Copyright (c) 2000-2022 the FFmpeg developers
  built with Apple clang version 14.0.0 (clang-1400.0.29.202)
  configuration: --prefix=/usr/local/Cellar/ffmpeg/5.1.2_4 --enable-shared --enable-pthreads --enable-version3 --cc=clang --host-cflags= --host-ldflags= --enable-ffplay --enable-gnutls --enable-gpl --enable-libaom --enable-libaribb24 --enable-libbluray --enable-libdav1d --enable-libmp3lame --enable-libopus --enable-librav1e --enable-librist --enable-librubberband --enable-libsnappy --enable-libsrt --enable-libsvtav1 --enable-libtesseract --enable-libtheora --enable-libvidstab --enable-libvmaf --enable-libvorbis --enable-libvpx --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxml2 --enable-libxvid --enable-lzma --enable-libfontconfig --enable-libfreetype --enable-frei0r --enable-libass --enable-libopencore-amrnb --enable-libopencore-amrwb --enable-libopenjpeg --enable-libspeex --enable-libsoxr --enable-libzmq --enable-libzimg --disable-libjack --disable-indev=jack --enable-videotoolbox
  libavutil      57. 28.100 / 57. 28.100
  libavcodec     59. 37.100 / 59. 37.100
  libavformat    59. 27.100 / 59. 27.100
  libavdevice    59.  7.100 / 59.  7.100
  libavfilter     8. 44.100 /  8. 44.100
  libswscale      6.  7.100 /  6.  7.100
  libswresample   4.  7.100 /  4.  7.100
  libpostproc    56.  6.100 / 56.  6.100
Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '/Users/nunatic/Dropbox/2023Spring/cs6682-SP2023/CS6682-SP2023-A1/part2/AbeVideos/AbeSelfIntroCrop_short.mp4':
  Metadata:
    major_brand     : mp42
    minor_version   : 512
    compatible_brands: mp42iso2avc1mp41
    creation_time   : 2023-02-13T23:30:06.000000Z
    encoder         : HandBrake 1.4.2 2021100300
  Duration: 00:00:05.04, start: 0.000000, bitrate: 1279 kb/s
  Stream #0:0[0x1](und): Video: h264 (Main) (avc1 / 0x31637661), yuv420p(tv, smpte170m/smpte170m/bt709, progressive), 620x680 [SAR 1:1 DAR 31:34], 1117 kb/s, 29.45 fps, 29.08 tbr, 90k tbn (default)
    Metadata:
      creation_time   : 2023-02-13T23:30:06.000000Z
      handler_name    : VideoHandler
      vendor_id       : [0][0][0][0]
  Stream #0:1[0x2](und): Audio: aac (LC) (mp4a / 0x6134706D), 44100 Hz, mono, fltp, 153 kb/s (default)
    Metadata:
      creation_time   : 2023-02-13T23:30:06.000000Z
      handler_name    : Mono
      vendor_id       : [0][0][0][0]
Stream mapping:
  Stream #0:0 -> #0:0 (h264 (native) -> rawvideo (native))
Press [q] to stop, [?] for help
Output #0, image2pipe, to 'pipe:':
  Metadata:
    major_brand     : mp42
    minor_version   : 512
    compatible_brands: mp42iso2avc1mp41
    encoder         : Lavf59.27.100
  Stream #0:0(und): Video: rawvideo (RGB[24] / 0x18424752), rgb24, 620x680 [SAR 1:1 DAR 31:34], q=2-31, 294276 kb/s, 29.08 fps, 29.08 tbn (default)
    Metadata:
      creation_time   : 2023-02-13T23:30:06.000000Z
      handler_name    : VideoHandler
      vendor_id       : [0][0][0][0]
      encoder         : Lavc59.37.100 rawvideo

videolines[0] will be Stream #0:0[0x1](und): Video: h264 (Main) (avc1 / 0x31637661), yuv420p(tv, smpte170m/smpte170m/bt709, progressive), 620x680 [SAR 1:1 DAR 31:34], 1117 kb/s, 29.45 fps, 29.08 tbr, 90k tbn (default)

videolines[-1] will be Stream #0:0(und): Video: rawvideo (RGB[24] / 0x18424752), rgb24, 620x680 [SAR 1:1 DAR 31:34], q=2-31, 294276 kb/s, 29.08 fps, 29.08 tbn (default)

meta["fps"] = fps will be set to the tbr value of the output video stream, namely 29.08. What we actually want is the fps value of the input video stream, namely 29.45.

Changes

1. change to be parsing data from the output video stream the input video stream

Because we are interating through (videolines[0], videolines[-1]), the fps value will always be parsing data from videolines[-1]. Namely, the output video stream. However, we should really be parsing data from the input video stream. So I've changed it to [videolines[0]]. A benefit for keeping the for line in [videolines[0]] structure, even when there's only one element in the array, is that in cases when we don't have any video stream, videolines[0] will be None, and the code will still work.

2. Change to remove the preference of tbr over fps

As I've described in https://github.com/imageio/imageio/issues/949, we should by default parse and use ffmpeg's fps, instead of tbr, as our fps metadata. Because the source code of imageio is using fps as average frame rate, namely total # of frames / total duration. And ffmpeg's fps value also denotes average frame rate, while tbr is just a guessed frame rate. If not doing so, get_data(index) will always return the wrong video frame data when fps and tbr of a video doesn't match. And even worse, trying run get_data of the last few frames will result in fatal IndexError.

So I've removed matches.sort(key=lambda x: x[1] == "tbr", reverse=True), because the function of this line is to prefer tbr over fps. And I've also removed matching for tbr, because we don't need that value. I don't think there's any cause where tbr data exists, while fps data is missing.

[almarklein]

I just merged a PR to update to the latest black to fix the linting errors. So you can merge or rebase with master to fix the Linting build.

Other than that, it looks like there are some tests with hard-coded fps numbers that need to be updated.

[Nunatic02]

I just merged a PR to update to the latest black to fix the linting errors. So you can merge or rebase with master to fix the Linting build.

Other than that, it looks like there are some tests with hard-coded fps numbers that need to be updated.

Wow cool! Thank you so much! Should I go ahead and merge master to my forked repo, and fix those test cases? I'm kind of nervous because I have never made changes to any open source packages before 🥺.

[almarklein]

Should I go ahead and merge master to my forked repo, and fix those test cases?

Yes, that'd be great 👍

I'm kind of nervous because I have never made changes to any open source packages before 🥺.

Really? I'm impressed by your thoroughness. If you need help with technical issues regarding git or something, feel free to ask for help!

[Nunatic02]

@almarklein Thank you so much Almar! You have made my day! 🥰

[almarklein]

I've forgotten about this, sorry!