Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TS] Segment template $Time$ is truncated to 32 bits #701

Closed
Canta opened this issue Jan 22, 2020 · 10 comments
Closed

[TS] Segment template $Time$ is truncated to 32 bits #701

Canta opened this issue Jan 22, 2020 · 10 comments
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@Canta
Copy link
Contributor

Canta commented Jan 22, 2020

System info

Operating System: CentOS Linux release 7.7.1908 (Core)
Shaka Packager Version: c731217

Issue and steps to reproduce the problem

Quick description:
When packager runs for a long enough time, $$Time$$ value resets and goes back from zero.

Packager Command:

shaka-packager 
'in=input, stream_selector=0,segment_template=/path/240_$$Time$$.ts,playlist_name=240.m3u8,drm_label=SD,bandwidth=512000' 
'in=input, stream_selector=1,segment_template=/path/360_$$Time$$.ts,playlist_name=360.m3u8,drm_label=SD,bandwidth=768000' 
'in=input, stream_selector=2,segment_template=/path/480_$$Time$$.ts,playlist_name=480.m3u8,drm_label=SD,bandwidth=1536000' 
'in=input, stream_selector=3,segment_template=/path/720_$$Time$$.ts,playlist_name=720.m3u8,drm_label=HD,bandwidth=3072000' 
'in=input, stream_selector=4,segment_template=/path/audio_$$Time$$.ts,playlist_name=audio.m3u8,drm_label=AUDIO,bandwidth=64000,language=eng' 
--hls_master_playlist_output /path/master.m3u8 
--hls_playlist_type LIVE 
--segment_duration 3.2 
--minimum_update_period 3.2
--time_shift_buffer_depth 7200 
--preserved_segments_outside_live_window 100 
--dump_stream_info 

What is the expected result?:
To increase linearly the value of $$Time$$

What happens instead?:
$$Time$$ resets its value.

Additional information:

Please take a look at this HLS medialist extract:

[user@server ~]# cat /path/audio.m3u8
(...)
#EXTINF:3.200,
audio_4294641887.ts
#EXTINF:3.200,
audio_4294929887.ts
#EXTINF:3.200,
audio_250591.ts
#EXTINF:3.200,
audio_538591.ts
#EXTINF:3.200,
audio_826591.ts
(...)

There, you can see a sudden time jump, from 4294929887 to 250591.
I believe what's happening is that this $$Time$$ value increased over the uint32 limit of 4294967295 (0xffffffff).

There's no error, nor any signal restart. Playback is fine. However, I do some other stuff with the chunks later, outside of shaka packager's scope. I need to have a linear incremental time-related filename for the chunks. The [rendition]_$$Time$$.[extension] pattern is great for me, but this overflow thing is giving me trouble.

I believe the most obvious fix would be to change that variable data type to something bigger. However, I'm also not sure what are my options. I'm feeding streams to shaka packager through UDP or pipes. They're MPEGTS streams. I could play with ffmpeg or any other tool in a way to handle that $$Time$$: let's say, for example, taking away a few orders of magnitude. As long as the result is linearly incremental, and has a time-based logic, I'm fine with it. But I'm not sure what parameter would let me exactly do something like that. I saw in the docs that it mentions printf formatting options, but the only example I see is how to put a minimum order of magnitude with padding, and such thing will not save me from this problem.

So, in this ticket I ask for one of two things:

  1. You think it's ok to change $$Time$$ data type to something bigger than uint32?
  2. If not, would you please give me some pointers in how to achieve the result I want? This could be from file naming guidelines to how to tune up packager's input so I can manipulate that value's order of magnitude.

Thanks.

@kqyang
Copy link
Collaborator

kqyang commented Jan 22, 2020

@Canta Thanks for the issue report. This looks like a bug to me as the timestamp should be 64 bits instead of 32 bits, i.e. it shouldn't wrap around at 0xffffffff.

Are you able to re-produce the problem? Can you increase verbose level to 4 (--v=4)? It can produce more debug which may give us a clue on where and why the timestamp is truncated?

@kqyang kqyang added the type: bug Something isn't working correctly label Jan 22, 2020
@kqyang kqyang added this to the v2.5 milestone Jan 22, 2020
@Canta
Copy link
Contributor Author

Canta commented Jan 22, 2020

Errr... I think that would be messy. If I did my math right (most likely I did not), it would take 1.7 days or so to reach that point from zero. All that time generating verbose logs will make it hard to handle, and I gotta guess when does the wrap actually happen.

No worries, I'll do it if it's needed to understand this. But would really like to avoid it. Isn't it any way to force the startup $$Time$$ value to some fixed number that would let us predict the wrap time? I'm thinking in something like using ffmpeg with some -output_ts_offset parameter as input, so then shaka packager starts from somewhere predictable and we can do a "trigger this problem in less than a minute" kind of test. But I don't quite understand very well MPEGTS timings yet and how shaka handles them.

I'll take a look at this, make some experiments. If no luck, I'll go your way and generate a huge log somewhere. It most likely will take me a day or two to set this up. Please let me know if you happen to know a better way to trigger this.

Thanks!

@kqyang
Copy link
Collaborator

kqyang commented Jan 22, 2020

@Canta I have not tried myself but I think you instruct ffmpeg to generate a stream with large time offsets. Shaka Packager does not change timestamps. The input timestamp will be carried as is to the output (unless it is truncated somewhere).

There are two possible causes of the problem:
(1) The timestamps in the input is already truncated
(2) It is truncated by Shaka Packager somewhere in the code

Let us know if you manage to create a source stream using ffmpeg with large timestamps. It will help us with the investigation.

it would take 1.7 days or so to reach that point from zero.

It should be 0.55 days if starting from 0: (1 << 32) / 90000.0 / 3600 / 24.

You may also use any of your existing live stream and let it run for 0.55 days.

@kqyang kqyang added needs triage and removed type: bug Something isn't working correctly labels Jan 22, 2020
@Canta
Copy link
Contributor Author

Canta commented Jan 24, 2020

Update:

I still didn't had the time to test it right, but I've seen something strange that I'd like to report here. I get an input stream, transcode it, and feed it to two different shaka packager instances: one for HLS, and another one for MPEG-DASH. This is because I need two different encryption setups for DRM. But both packager instances receives the same (splitted) input: no changes in timestamps at all between them.

The weird thing I see is that MPEG-DASH doesn't show this problem. DASH chunks are generated fine with times like 8641728215, 8642016215, and so on. It behaves as expected.

Thing is, I'm using my fork of shaka packager discussed in #691, which indeed deals with HLS mechanics. Could it be related?

In fact, I have this concrete question: is the media_sequence_number used to calculate that $Time$ value in HLS logic? Because I've changed it from int to uint32_t, as big sequence numbers turned out to be negative in tests. But now I'm suddenly dealing with a uint32_t limit somewhere else.

Shouldn't be the case, as otherwise it would give me negative numbers given the same situation. But It's the only thing I can't think of here. Perhaps I triggered another hidden bug somehow by my modifications?

@kqyang
Copy link
Collaborator

kqyang commented Jan 24, 2020

Ahh, that is good information. No, it is not because of your change, but your data does help us pinpoint where the problem is:

https://github.com/google/shaka-packager/blob/master/packager/media/formats/mp2t/ts_segmenter.cc#L128

diff --git i/packager/media/formats/mp2t/ts_segmenter.cc w/packager/media/formats/mp2t/ts_segmenter.cc
index 3474ec1a..aaea5ced 100644
--- i/packager/media/formats/mp2t/ts_segmenter.cc
+++ w/packager/media/formats/mp2t/ts_segmenter.cc
@@ -125,7 +125,7 @@ void TsSegmenter::SetTsWriterFileOpenedForTesting(bool value) {
   ts_writer_file_opened_ = value;
 }
 
-Status TsSegmenter::OpenNewSegmentIfClosed(uint32_t next_pts) {
+Status TsSegmenter::OpenNewSegmentIfClosed(int64_t next_pts) {
   if (ts_writer_file_opened_)
     return Status::OK;
   const std::string segment_name =
diff --git i/packager/media/formats/mp2t/ts_segmenter.h w/packager/media/formats/mp2t/ts_segmenter.h
index 78a88d22..b140040e 100644
--- i/packager/media/formats/mp2t/ts_segmenter.h
+++ w/packager/media/formats/mp2t/ts_segmenter.h
@@ -71,7 +71,7 @@ class TsSegmenter {
   void SetTsWriterFileOpenedForTesting(bool value);
 
  private:
-  Status OpenNewSegmentIfClosed(uint32_t next_pts);
+  Status OpenNewSegmentIfClosed(int64_t next_pts);
 
   // Writes PES packets (carried in TsPackets) to a file. If a file is not open,
   // it will open one. This will not close the file.

Can you try if the above change fixes your problem? If yes, you are welcome to send us a pull request?

Thanks.

@Canta
Copy link
Contributor Author

Canta commented Jan 24, 2020

The TS format! That's something I started using about a week ago because of some devices. When I used fmp4 I never saw this happening, and I'm using it only for HLS. Makes sense.

I've patched this by hand in one of our servers, and will let it run the whole weekend on a single stream. Most likely will have feedback by next monday.

BTW, two questions:

  1. I see this other suspiciously-related-by-name uint32_t var. Is it ok for it to be 32 bits?
  2. Is it ok to use int64_t instead of uint64_t for OpenNewSegmentIfClosed? Do you expect negative numbers there?

Thanks a lot.

@kqyang
Copy link
Collaborator

kqyang commented Jan 24, 2020

Is it ok to use int64_t instead of uint64_t for OpenNewSegmentIfClosed? Do you expect negative numbers there?

Great.

I see this other suspiciously-related-by-name uint32_t var. Is it ok for it to be 32 bits?

Yes, it is a user set offset. We don't expect it to be more than (1 << 32) / 90000 seconds.

Is it ok to use int64_t instead of uint64_t for OpenNewSegmentIfClosed? Do you expect negative numbers there?

Yes, please use int64_t for consistency. And yes, it is signed so it can handle negative numbers in some situations.

@Canta
Copy link
Contributor Author

Canta commented Jan 25, 2020

It worked. Now I see ts segments with $Time$ way over the uint32 limit, and also in sync with the MPEG-DASH chunks timings.

Let me first finish the #691 PR, and then will send you another one for this. It's an easy fix.

@kqyang
Copy link
Collaborator

kqyang commented Jan 28, 2020

@Canta Great. Thanks for the confirmation!

@kqyang kqyang changed the title Problem with $$Time$$ value limit [TS] Segment template $Time$ is truncated to 32 bits Jan 28, 2020
@kqyang kqyang added type: bug Something isn't working correctly and removed needs triage labels Jan 31, 2020
Canta pushed a commit to Canta/shaka-packager that referenced this issue Jan 31, 2020
Also, added Daniel Cantarín to AUTHORS and CONTRIBUTORS files, as per @kqyang comment: shaka-project#702 (comment)
@kqyang kqyang closed this as completed in 1ed7de2 Feb 2, 2020
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 2, 2020
@shaka-project shaka-project locked and limited conversation to collaborators Apr 2, 2020
@kqyang
Copy link
Collaborator

kqyang commented Aug 1, 2020

The fix was cherry-picked to v2.4.2.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

3 participants