Page MenuHome

Fix T88623, T87044: Make encoded videos play correctly in VLC
ClosedPublic

Authored by Sebastian Parborg (zeddb) on May 28 2021, 4:25 PM.

Details

Summary

The issue was two fold. We didn't properly:

  1. Initialize the codec default values which would lead to VLC complaining because of garbage/wrong codec settings.
  1. Calculate the time base for the video. FFmpeg would happily accept this but VLC seems to assume the time base value is at least somewhat correct and couldn't properly display the frames as the internal time base was huge. We are talking about 90k ticks (tbn) for one second of video.

This patch incorporates the changes from https://developer.blender.org/D10988 and fixes the time base calculation so it follows the guidelines from ffmpeg.

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.May 28 2021, 4:25 PM
Sebastian Parborg (zeddb) created this revision.

Does this patch not work without changes in D10988? Not sure if these would be better to commit separately. D10988 has broader implications it seems.

Also I would probably move this to separate function. Possibly add example that 3/0.1 is converted to 30/1 so it doesn't have to be analyzed too closely if it works as expected? Perhaps it's just me not used to math in practice :)

/* Convert the fps base to an integer. Simply shift the decimal places until we get an integer
 * (within a floating point error range). */
unsigned int den = rd->frs_sec;
double num = rd->frs_sec_base;
float eps = FLT_EPSILON;
unsigned int DENUM_MAX = (codec_id == AV_CODEC_ID_MPEG4) ? (1UL << 16) - 1 : (1UL << 31) - 1;

/* Calculate the precision of the initial floating point number. */
if (num > 1.0) {
  unsigned int integer_bits = log2_floor_u((unsigned int)num);

  /* Formula for calculating the epsilon value: (power of two range) / (pow mantissa bits)
   * For example, a float has 23 manitissa bits and 3.5f as a pow2 range of (4-2):
   * (4-2) / pow2(23) = floating point precision
   */
  eps = (float)(1 << integer_bits) * FLT_EPSILON;
}

/* Calculate how many decimal shifts we can do until we run out of precision. */
int max_num_shift = fabsf(log10f(eps));
/* Calculate how many times we can shift the denominator. */
int max_den_shift = log10f(DENUM_MAX) - log10f(den);
int max_iter = min_ii(max_num_shift, max_den_shift);

for (int i = 0; i < max_iter && fabs(num - round(num)) > eps; i++) {
  /* Increase the number and denominator until both are integers. */
  num *= 10;
  den *= 10;
  eps *= 10;
}

Otherwise LGTM.

Does this patch not work without changes in D10988? Not sure if these would be better to commit separately. D10988 has broader implications it seems.

I basically took the changes from D10988 and updated them. All of it is in this patch, so you don't need to apply it.

I incorporated the changes into this patch as simply updating the time base calculations didn't make VLC happy.
The "initialize defaults" also makes mpv happy as it would complain that there were garbage frames at the end of the video before.
(but it would playback the video seemingly correctly)

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)

Moved out the timebase calculation into a function.

This revision is now accepted and ready to land.May 28 2021, 6:49 PM

Not really my area of expertise. There are some inlined comment which would be nice to have (more const, more clear that something is a number of things). But this is more of pedantic changes.
Really like that there are links and explanations of formulas now!

If it works for you then just go ahead and commit.

source/blender/blenkernel/intern/writeffmpeg.c
533
543