Opened 11 years ago

Closed 8 years ago

#2828 closed defect (invalid)

HLS segment MPEGTS continuity counter is being incorrectly set to 0 on each segment

Reported by: dlevinson5 Owned by:
Priority: normal Component: avformat
Version: unspecified Keywords: hls segment mpegts
Cc: Doychin Dokov, mikie@iki.fi Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Summary of the bug:

when creating hls segments using ffmpeg the continuity counter is being reset to 0 for each segment instead of continuing from the prior segment.

This is somewhat critical since this breaks the MPEG2 specification and does not pass Apple's HLS compliance requirements.

In the mpegts_write_header() function the pat, sdt and st_ts->cc counters are reset when the segment is written instead of being carried forward from the prior segment.

To work around this issue I created several static variables to persist the continuity counters and added a flag to switch between preserving continuity or using the default logic.

static int mpegts_pat = 15;
static int mpegts_sdt = 15;
static int mpegts_cc[10] = {15,15,15,15,15,15,15};

How to reproduce:

%ffmpeg -i <source> -c copy -map 0 -vbsf h264_mp4toannexb -initial_offset 10 -segment_list_type m3u8 -flags +global_header -f segment -segment_list_flags +live -segment_list_type m3u8 -segment_list \"{1}.m3u8\" -segment_time 10 -segment_format mpegts <output>_%d.ts

Inspect each segment continuity counter values in sequence and it will be observed that the continuity counter is reset to 0 at the start of each segment. 

ffmpeg version v2.0 /  N-54901-g55db06a / Win32/64



Attachments (2)

mpegtsenc.c (42.4 KB ) - added by dlevinson5 11 years ago.
modified mpegtsenc.c file
mpegtsenc-cckeep.patch (2.8 KB ) - added by Doychin Dokov 10 years ago.
Rewrite of the patch. Cleanup code and rename option to "cckeep"

Download all attachments as: .zip

Change History (31)

by dlevinson5, 11 years ago

Attachment: mpegtsenc.c added

modified mpegtsenc.c file

comment:1 by Carl Eugen Hoyos, 11 years ago

Keywords: hls mpegts added; HLS MPEGTS removed
Priority: criticalnormal

Please provide an actual failing command line together with the complete, uncut console output to make this a valid ticket.
If you have a patch fixing the problem, please send it (with git format-patch or as unified diff) to the ffmpeg-devel mailing list.

comment:2 by Carl Eugen Hoyos, 11 years ago

Analyzed by developer: unset
Reproduced by developer: unset

comment:3 by dicroce, 11 years ago

Any progress on this? I verified it this morning with dvbsnoop that continuity counters are 0 for each file. VLC shows "ts warning: discontinuity received 0x0 instead of" in its log when playing these stream (there is also macro blocking on iOS devices at the same moment, and I'm hoping they are related).

comment:4 by Carl Eugen Hoyos, 11 years ago

If you want to help, please provide a simple command line - preferably with -f lavfi -i testsrc and without using an external library if possible - together with the complete, uncut console output and explain how the problem can be verified.

Last edited 11 years ago by Carl Eugen Hoyos (previous) (diff)

comment:5 by dicroce, 11 years ago

The problem is the bug does not result in a failure of the ffmpeg tool. It splits the files, and the files are even playable. The problem is that for clients to play smoothly from file to file certain counters in the ts headers are supposed to increment linearly across the sequence of files. In the current code, they are always 0. The failure to increment these counters results in a small macroblock effect on iOS devices.

Further, I am not actually using the ffmpeg tool. I am writing software and using libavcodec and libavformat directly. I am not actually that familiar with how to tell the ffmpeg binary to segment files.... Tho, I can tell you how how to do it from a C program.

The original posters attached "mpegtsenc.c" file does actually fix the problem (if you enable his fix via AVOptions). The problem is that it uses 3 static globals to preserve the counters from call to call. I suspect this code will fail horribly in a multithreaded environment.

comment:6 by dlevinson5, 11 years ago

I have a feeling this issue will never be corrected. I have provided a command-line sample on how to generate segments, provided the active functions and code locations for the issue and also provided a work around.

My work around is certainly a hack and NOT thread safe. The parameters need to be added to a context. Given that each segment is treated as a separate file encoding with a different mark in/out point I could not find a simple way to pass the counter between encodings. I needed a quick fix and the static variable works since I am using ffmpeg in a command-line situation. I suppose I could have added the value to the AVContext which would be cleaner.

in reply to:  6 comment:7 by dicroce, 11 years ago

Replying to dlevinson5:

I have a feeling this issue will never be corrected. I have provided a command-line sample on how to generate segments, provided the active functions and code locations for the issue and also provided a work around.

My work around is certainly a hack and NOT thread safe. The parameters need to be added to a context. Given that each segment is treated as a separate file encoding with a different mark in/out point I could not find a simple way to pass the counter between encodings. I needed a quick fix and the static variable works since I am using ffmpeg in a command-line situation. I suppose I could have added the value to the AVContext which would be cleaner.

I totally appreciate your work on this. It gets me a little farther down the road! The issue is that I'm going to need a more permanent solution. I don't really mind applying a patch on top of ffmpeg (if they won't fix this), but it does need to be thread safe. I'll take a look at adding your statics to AVContext this weekend.

comment:8 by Carl Eugen Hoyos, 11 years ago

Please understand that if you want this problem fixed, two things will certainly help:

  • A simple command line that allows to produce such a "bad" stream together with complete, uncut console output, if possible without using external libraries (ie without using libx264) and using internal source (-f lavfi -i testsrc). Both make testing very significantly easier.
  • Steps how to reproduce that the resulting output is "bad".

On a general note, please understand that while an analysis of the problem is nice, it is certainly not (never) required and it definitely should never replace the actual report (that contains a simple command line, complete, uncut console output and a short explanation what the problem is or how the problem can be seen). Patches are even better but please do not send them to this bug tracker, except if you just want to test them.

comment:9 by dlevinson5, 11 years ago

=> command-line to replicate (as noted in the original report)

ffmpeg -i <source> -c copy -map 0 -vbsf h264_mp4toannexb -initial_offset 10 -segment_list_type m3u8 -flags +global_header -f segment -segment_list_flags +live -segment_list_type m3u8 -segment_list \"{1}.m3u8\" -segment_time 10 -segment_format mpegts <output>_%d.ts

=> version of ffmpeg used => fmpeg version v2.0 / N-54901-g55db06a / Win32/64

=> to validate the segment continuity I used tstools available from the link below. I used the tsreport -cnt <pid> <.ts file> which generates a continuity counter file. If you run this for each segment created from the prior command like you will see that each starts at 0 when it should continue from the prior segment which is the same as if you I was to use ffmpeg to create a full length mpeg2 file.

http://code.google.com/p/tstools/

You could also use ffprobe to dump out the continuity counter for each frame but you would need to modify the ffprobe source to output this value since I could not find a way to do this via the command-line.

=> This issue was raised by Apple engineers and from what I understand ffmpeg does not produce HLS complaint iOS content due to this break in the mpeg-ts specification.

comment:10 by Carl Eugen Hoyos, 11 years ago

You seem to believe that I cannot read: That is - afaict - not the case.

Does the following allow to reproduce the problem?

$ ffmpeg -f lavfi -i testsrc -t 120 out.m3u8

If yes, please explain how to use tsreport, dvbsnoop or vlc to verify the problem. If not, try to add the absolutely necessary options.

Generally, please do not post command lines that contain variables, always post actual command lines, and always post the complete, uncut console output together with the command line. If a problem is reproducible without x264, don't use it, if x264 is needed, add a short explanation why.

I don't know how difficult it will be to export the continuity counter from the mpegts muxer to the hls muxer and to set the mpegts continuity counter from the hls muxer but I guess new functions ff_get_continuity_counter() and ff_set_continuity_counter() should allow that. In any case, please try to understand that it will take time to implement it, since time is the only limiting factor in FFmpeg development, I suggest you try hard to reduce the time by providing (very simple) fool-proof explanations how to reproduce the problem (for any ticket).

in reply to:  1 comment:11 by Stefano Sabatini, 11 years ago

Replying to cehoyos:

Please provide an actual failing command line together with the complete, uncut console output to make this a valid ticket.
If you have a patch fixing the problem, please send it (with git format-patch or as unified diff) to the ffmpeg-devel mailing list.

+1 for the patch. The whole C file is not useful, since we can't spot the difference with your reference, nor it is easy to check the difference with the actual version since other unrelated changes may be present in the new version.

comment:12 by dicroce, 11 years ago

I have extracted your changes from your original mpegtsenc.c... Once I did that it was pretty straightforward to move the counters into AVFormatContext, which appears to have totally fixed the problem. I'd offer up the patch files, but something tells me the community might not be too thrilled by 3 new members for AVFormatContext just to support mpegts files.

On the other hand, if ff_get_continuity_counter() and ff_set_continuity_counter() actually existed, that might be a cleaner solution for everyone...

in reply to:  12 ; comment:13 by jaanusk, 11 years ago

Replying to dicroce:

I have extracted your changes from your original mpegtsenc.c... Once I did that it was pretty straightforward to move the counters into AVFormatContext, which appears to have totally fixed the problem. I'd offer up the patch files, but something tells me the community might not be too thrilled by 3 new members for AVFormatContext just to support mpegts files.

On the other hand, if ff_get_continuity_counter() and ff_set_continuity_counter() actually existed, that might be a cleaner solution for everyone...

Care to share your patch?

in reply to:  13 comment:14 by Carl Eugen Hoyos, 11 years ago

Replying to jaanusk:

Care to share your patch?

Can you describe how to reproduce the problem? Is it reproducible with the command in comment:10?

comment:15 by dlevinson5, 11 years ago

Using "ffmpeg -i big_buck_bunny_480p_h264.mov -t 120 output.m3u8" does produce .ts segments with the correct continuity counter.

Using "ffmpeg -i "big_buck_bunny_480p_h264.mov" -c copy -map 0 -vbsf h264_mp4toannexb -f segment -segment_time 10 -segment_list_type m3u8 -segment_list index.m3u8 -segment_format mpegts segment_%d.ts" reproducts the issue where the continuity counter it reset to 0.

You can inspect the continuity counters using the following command tsreport command from the tstools package available from Google Code (http://code.google.com/p/tstools/).

(correctly shows the counters in each filing picking up where it left off in the prior file)

tsreport -cnt 256 "output1.ts"
type continuity_counter.txt

tsreport -cnt 256 "output2.ts"
type continuity_counter.txt

(incorrectly shows the counters resetting to 0 on each file)

tsreport -cnt 256 "segment_0.ts"
type continuity_counter.txt

tsreport -cnt 256 "segment_1.ts"
type continuity_counter.txt


comment:16 by Jeff Johnson, 10 years ago

I am having the same problem. I am also not using the ffmpeg tool, but the libraries directly, but the problem can easily be made to show itself using the tool as well:

Here are unredacted commands and outputs.

(Output too big to post here when trying to submit, so here is pastebin):
$ ffmpeg -i http://download.wavetlan.com/SVV/Media/HTTP/MP4/ConvertedFiles/Media-Convert/Media-Convert_test1_36s_AVC_VBR_521kbps_320x240_25fps_AACLCv4_VBR_96kbps_Stereo_44100Hz.mp4 -c copy -map 0:v -map 0:a -bsf h264_mp4toannexb -flags -global_header -segment_list_flags +live-cache -segment_time 2 -segment_list_size 30 -segment_wrap 60 -segment_time_delta 0.08 -segment_list ./test.m3u8 -f ssegment "./test%05d.ts" -report

http://pastebin.com/aScZgCnN

Now let's read it back:

(Output too big to post here when trying to submit, so here is pastebin, again):

$ ffmpeg -debug 1 -i test.m3u8 -f null /dev/null -report

http://pastebin.com/kyd6k6rt

As you can see, at the start of each segment, there are failed continuity checks as the timestamp is incorrectly reset at the start of each new segment:

(Excerpt from above)

[hls,applehttp @ 0x1b54a20] HLS request for url 'test00003.ts', offset 0, playlist 0

[mpegts @ 0x1b573e0] Continuity check failed for pid 17 expected 5 got 0
[mpegts @ 0x1b573e0] Continuity check failed for pid 0 expected 8 got 0
[mpegts @ 0x1b573e0] Continuity check failed for pid 4096 expected 8 got 0
[mpegts @ 0x1b573e0] Continuity check failed for pid 256 expected 7 got 0

This will cause the output to fail Apple validation and is breaking specification, so technically the output is corrupted. While the output does play in ffmpeg, VLC, ffplay, etc, but gives warnings similar to above in all cases.

comment:17 by Jeff Johnson, 10 years ago

Interesting, according to a W3C document at https://dvcs.w3.org/hg/html-media/raw-file/d5956e93b991/media-source/media-source.html#mpeg2ts-discontinuities it is stated that clients should handle TS discontinuities:

• When a discontinuity is detected, MPEG2TS_timestampOffset must be adjusted to make the timestamps after the discontinuity appear to come immediately after the timestamps before the discontinuity.

However, the Apple documentation for the validation tool at https://developer.apple.com/library/ios/technotes/tn2235/_index.html#//apple_ref/doc/uid/DTS40010221 specifies:

WARNING: stream discontinuity detected

You must use the EXT-X-DISCONTINUITY tag where an encoding discontinuity exists between the
media file that follows it and the one that preceded it. See the IETF Internet Draft of the
HTTP Live Streaming Protocol Specification for more information.

If you intend to play a new stream at a given point in your presentation, you must use the
EXT-X-DISCONTINUITY tag.

So a workaround that would be a quick hackish would might be to change the M3U8 output to include the EXT-X-DISCONTINUITY tag between all segments and should result in ffmpeg output passing the Apple validation tests.

The proper fix is still maintaining the timestamp across segments.

in reply to:  12 comment:18 by Jeff Johnson, 10 years ago

Replying to dicroce:

I have extracted your changes from your original mpegtsenc.c...
[snip]
On the other hand, if ff_get_continuity_counter() and ff_set_continuity_counter() actually existed, that might be a cleaner solution for everyone...

dicroce, if you are still watching and care to post the patch here, it would save me some time. If not I'll can work up something quickly myself. I'm hoping that a "real" ffmpeg developer would look into this first, because my unfamiliarity with the ffmpeg internals means any patch I submit would be less likely to be accepted than one by another developer more versed with the code.

by Doychin Dokov, 10 years ago

Attachment: mpegtsenc-cckeep.patch added

Rewrite of the patch. Cleanup code and rename option to "cckeep"

comment:19 by Doychin Dokov, 10 years ago

I've uploaded a patch against current git which does the idea of the original patch author.

Because counters are kept in global statics, it's not thread-safe and will probably fail in any scenarios other than standalone usage of ffmpeg with only one output per process. I'll have a look at the segmenter code as well to see which structure it would be appropriate to hold the counters into, in order to do this properly. Anyone who has a better inside of ffmpeg - please do share your suggestions / ideas. / opinions.

in reply to:  19 comment:20 by Carl Eugen Hoyos, 10 years ago

Cc: Doychin Dokov added

Replying to lancey:

Because counters are kept in global statics

This means that the patch unfortunately cannot be applied;(

Can you explain how I can reproduce the issue with testsrc input and without using external libraries?

comment:21 by Doychin Dokov, 10 years ago

Well, as I said these counters need to be moved to some structure related to the output, which is kept across segments, so there are copies per output.

You can reproduce the issue with the following command:

ffmpeg -i test.ts -codec copy -flags global_header -map 0 -f segment -segment_time 10 -segment_list_size 6 -segment_wrap 20 -segment_list playlist.m3u8 pl%03d.ts

This will output one ts file per 10 sec. Inspecting each ts file you'll see CC counters for each PID start from 0, instead of the appropriate number which the last PID packet had within the previous segment. I've personally not encountered a scenario where this breaks playout on any device/player, but some people do. Also, playling the result with VLC gives warnings as well (but no playout disturbances occur). Per HLS specs, counters should indeed be kept across segments, so we should fix this for sure.

Also, it would be even better if we keep the incoming CCs in case we are copying the stream.

If you need example files, both input and output, let me know.

comment:22 by Carl Eugen Hoyos, 10 years ago

So the issue is not reproducible using the testsrc input?

comment:23 by Doychin Dokov, 10 years ago

I guess it would be, but i don't think it produces a ts stream, and you'll have to encode it as well - and you said you don't want any external libraries involved. So, the simplest test is to just try to slice some .ts file.

comment:24 by Carl Eugen Hoyos, 10 years ago

What I meant was: Is the issue also reproducible using testsrc input and encoding with -vcodec mpeg2video?

comment:25 by Doychin Dokov, 10 years ago

Yes, it absolutely is. Command to reproduce:

ffmpeg -re -f lavfi -i testsrc -vcodec mpeg2video -flags global_header -map 0 -f segment -segment_time 10 -segment_list_size 6 -segment_wrap 20 -segment_list pl.m3u8 pl%03d.ts

Each produced .ts has the CC counters of each PID start from 0.

I'll publish a revised patch tomorrow.

comment:26 by Mika Raento, 10 years ago

Cc: mikie@iki.fi added

comment:27 by Mika Raento, 10 years ago

For HLS you are probably better off using the hls muxer instead of the generic segment muxer - it will take care of making HLS-compatible TS files. See https://www.ffmpeg.org/ffmpeg-formats.html#hls-1

comment:28 by Michael Niedermayer, 8 years ago

this is fixed when using "-individual_header_trailer 0"

comment:29 by Carl Eugen Hoyos, 8 years ago

Keywords: segment added
Resolution: invalid
Status: newclosed

This ticket seems to be a big misunderstanding:
If you need Apple-compatible hls streams use the hls muxer. If you use the segment muxer, the streams will not (necessarily) be hls-compatible streams.
You can use the option individual_header_trailer to set if you want individual headers with certain values reset for each segment.

Note: See TracTickets for help on using tickets.