Always set ffmpeg flag +genpts when video stream is being copied #977

Lynxy · 2019-02-22T01:10:11Z

Changes
Ensure ffmpeg flag +genpts is set if target codec is copy.

Issues
Transcoding fails on video streams that lack PTS data. When ffmpeg is copying a video stream which lacks PTS data or for some other reason PTS is not passed along correctly, the ffmpeg decoder is bypassed and PTS data is not ever generated, resulting in a stream that never plays.

Output of ffmpeg in such a case:

[NULL @ 0x5bfc8c0] Failed to parse extradata
[segment @ 0x5c20900] Timestamps are unset in a packet for stream 0. This is deprecated and will stop working in the future. Fix your code to set the timestamps properly
[mpegts @ 0x5c2b140] Timestamps are unset in a packet for stream 0. This is deprecated and will stop working in the future. Fix your code to set the timestamps properly
[mpegts @ 0x5c2b140] first pts value must be set
av_interleaved_write_frame(): Invalid data found when processing input
[mpegts @ 0x5c2b140] first pts value must be set
[segment @ 0x5c20900] Opening '/var/lib/jellyfin/transcoding-temp/e0cc15376cab26c8563e0be01117a4cd.m3u8.tmp' for writing
Error writing trailer of /var/lib/jellyfin/transcoding-temp/e0cc15376cab26c8563e0be01117a4cd%d.ts: Invalid data found when processing input

The +genpts flag will generate missing PTS if DTS is present. Relevant Stack Exchange answer: https://superuser.com/questions/710008/how-to-get-rid-of-ffmpeg-pts-has-no-value-error

cvium · 2019-02-22T05:10:33Z

Would be helpful if you could paste the actual command before and after the changes.

Also, I'm not convinced this actually places the flag before -i, which according to the ffmpeg devs is necessary.

JustAMan

Changes look okay-ish, but I second @cvium - please provide pre- and post-change ffmpeg command lines.

Lynxy · 2019-02-22T13:04:09Z

Sure thing. Here are the commands taken from /var/log/jellyfin/ (with media path replaced for privacy).

Before patch:

/usr/local/bin/ffmpeg-4.0.3-64bit-static/ffmpeg -f avi -i file:"/path/to/media/file.avi" -map_metadata -1 -map_chapters -1 -threads 8 -map 0:0 -map 0:1 -map -0:s -codec:v:0 copy -copyts -vsync -1 -codec:a:0 aac -strict experimental -ac 2 -ab 117512 -f segment -max_delay 5000000 -avoid_negative_ts disabled -start_at_zero -segment_time 6 -individual_header_trailer 0 -segment_format mpegts -segment_list_type m3u8 -segment_start_number 0 -segment_list "/var/lib/jellyfin/transcoding-temp/48b5614998dd85b101d4612dcd7ec15a.m3u8" -y "/var/lib/jellyfin/transcoding-temp/48b5614998dd85b101d4612dcd7ec15a%d.ts"

After patch:

/usr/local/bin/ffmpeg-4.0.3-64bit-static/ffmpeg -fflags +genpts -f avi -i file:"/path/to/media/file.avi" -map_metadata -1 -map_chapters -1 -threads 8 -map 0:0 -map 0:1 -map -0:s -codec:v:0 copy -copyts -vsync -1 -codec:a:0 aac -strict experimental -ac 2 -ab 117512 -f segment -max_delay 5000000 -avoid_negative_ts disabled -start_at_zero -segment_time 6 -individual_header_trailer 0 -segment_format mpegts -segment_list_type m3u8 -segment_start_number 0 -segment_list "/var/lib/jellyfin/transcoding-temp/f5f73d0b4df185cb317b36fc1b98cf.m3u8" -y "/var/lib/jellyfin/transcoding-temp/f5f73d0b4df185cb317b36fc1b98cf%d.ts"

I believe the flags are placed in the correct location because of the code at MediaBrowser.Api/Playback/Hls/BaseHlsService.cs:268. Under this line it even asks the question if +genpts should be added while copying.

cvium · 2019-02-22T19:41:53Z

Could you also fix these?

Lynxy · 2019-02-22T21:48:16Z

@cvium, this pull requests addresses EncodingHelper.cs:2453; it is moved up to line 2439 to read clearer.

As for EncodedRecorder.cs:152, what exactly needs fixing? I do not use the Live TV feature so I would not be able to test any changes, so I wouldn't feel comfortable doing any work to that.

JustAMan

LGTM.
Please add yourself to CONTRIBUTORS.md!

Lynxy · 2019-02-23T04:31:09Z

Hah alright, will do!

cvium · 2019-02-23T06:16:18Z

@cvium, this pull requests addresses EncodingHelper.cs:2453; it is moved up to line 2439 to read clearer.

As for EncodedRecorder.cs:152, what exactly needs fixing? I do not use the Live TV feature so I would not be able to test any changes, so I wouldn't feel comfortable doing any work to that.

The problem with EncodingHelper.cs:2453 is that the function is called here

jellyfin/MediaBrowser.Controller/MediaEncoding/EncodingHelper.cs

Line 2403 in 1ea219b

    
           GetProgressiveVideoArguments(state, encodingOptions, videoCodec, defaultH264Preset),

, which is after the -i flag.

Regarding EncodedRecorder.cs:152, it's a similar problem in that it's used here after -i:

jellyfin/Emby.Server.Implementations/LiveTv/EmbyTV/EncodedRecorder.cs

Line 203 in a6a4cd5

    
           var commandLineArgs = string.Format("-i \"{0}\"{5} {2} -map_metadata -1 -threads 0 {3}{4}{6} -y \"{1}\"",

Lynxy · 2019-02-23T14:51:32Z

@cvium, I provided a before/after log showing the issued ffmpeg command having +genpts before the -i switch as you requested. I would be happy to provide a .avi file that does not transcode without +genpts so you could test this patch yourself.

Here is how +genpts gets before -i.

EncodingHelper.cs:2453 is called from 4 locations:

MediaBrowser.Api.Playback.Hls.BaseHlsService.GetCommandLineArguments()
MediaBrowser.Api.Playback.Hls.DynamicHlsService.GetCommandLineArguments()
MediaBrowser.Controller.MediaEncoding.EncodingHelper.GetProgressiveAudioFullCommandLine()
MediaBrowser.Controller.MediaEncoding.EncodingHelper.GetProgressiveVideoFullCommandLine()

Number 1 and 2 from my list are similar so we'll examine those together. Look at the return string.Format() call in both of them. It formats the string with {0} coming first, which is inputModifier, a variable containing the output of EncodingHelper.GetInputModifier(), the function I modified. Next up is {1}, which is the output from EncodingHelper.GetInputArgument; it is within this function that the -i switch is added. It is quite clear how +genpts ends up before -i here.

Number 3 are 4 are similar as well. The return string.Format() call formats with {0} first, which is inputModifier, a variable containing the output of EncodingHelper.GetInputModifier(), the function I modified. Next comes {1} which is the output from this.GetInputArgument() which provides the -i switch. In these cases it is clear to see how +genpts ends up before -i.

You are right about the call to GetProgressiveVideoArguments() coming after the -i switch so I will revert the code there as that seems be intended to apply to the output file.

Lynxy · 2019-02-23T14:55:41Z

In regards to @cvium's second point, I do not know why you are asking me to fix EncodedRecorder.cs:152. This patch does not touch that file at all. The code you are referencing is existing code; Jellyfin will behave exactly the same with or without my patch. It is also for a completely different aspect (LiveTV) so should probably be it's own PR.

joshuaboniface · 2019-02-27T05:39:38Z

@cvium Does @Lynxy's last commit address your concerns?

joshuaboniface

Approved pending feedback from @cvium.

cvium · 2019-02-27T05:48:49Z

No, fflags only works on inputs according to the response we got from the ffmpeg mailing list. But it's fine, we'll deal with it separately.

cvium

Approved

Always set ffmpeg flag +genpts when video stream is being copied

9b39404

JustAMan reviewed Feb 22, 2019

View reviewed changes

JustAMan approved these changes Feb 22, 2019

View reviewed changes

Add to contributors

eb95b02

Leave +genpts untouched in GetProgressiveVideoArguments()

67f399d

joshuaboniface approved these changes Feb 27, 2019

View reviewed changes

cvium approved these changes Feb 27, 2019

View reviewed changes

joshuaboniface merged commit 9651a78 into jellyfin:master Feb 27, 2019

Lynxy mentioned this pull request Feb 28, 2019

incorrect use of ffmpeg options for remux #894

Closed

brianjmurrell added a commit to brianjmurrell/jellyfin that referenced this pull request Feb 28, 2019

Add patch for jellyfin#977 to Fedora RPM build

66e1b8f

brianjmurrell mentioned this pull request Mar 1, 2019

Fails to transcode to Chromecastable video codec #1027

Closed

brianjmurrell added a commit to brianjmurrell/jellyfin that referenced this pull request Mar 1, 2019

Add patch for jellyfin#977 to Fedora RPM build

351a887

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always set ffmpeg flag +genpts when video stream is being copied #977

Always set ffmpeg flag +genpts when video stream is being copied #977

Always set ffmpeg flag +genpts when video stream is being copied #977

Always set ffmpeg flag +genpts when video stream is being copied #977

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment