Skip to content

Conversation

@Billy2011
Copy link
Contributor

@Billy2011 Billy2011 commented Dec 9, 2020

you can test it with a swiss ip and this link.
closes #3328

total_duration = 0
while not self.closed:
for sequence in filter(self.valid_sequence, self.playlist_sequences):
if self.init_map != sequence.segment.map:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a bit of a comment describing the mechanism might be useful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map segment has to be put in the queue only once, and is then valid for all following segments until an ext_x_discontinuity tag arrives (in this case, a possibly new map segment follows) or the stream ends.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to save the init maps in a separate list and not in each Segment as before, because I want only the index of the init. map store, so less memory is used, especially with very long playlist of vod’s - do you have any objections, @beardypig, @bastimeyer?

store init maps in a separate list
Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HLSStreamWorker.iter_segments() should not create new segments and yield them with the same num. This is bad, because first, it's not the job of the worker to create segments, it's the job of the parser, and second, you're re-downloading the same media initialization over and over for each segment that comes after the EXT-X-MAP tag, which is wasteful.

Also not sure what the purpose of the init_map and maps list is. Each Segment's map attribute gets set on all segments following the EXT-X-MAP, as defined by the HLS spec:

A much better implementation would be fetching the media initialization once and writing it to the buffer in the HLSStreamWriter.write method before it writes the fetched segment data. The map-fetching would require some modifications of the writer's fetch method and making it so that the media initialization data of the map attribute gets stored somewhere and shared between the segments with the same map.

Proper tests are also a requirement before any feature extensions to the streaming classes can get merged, ideally with full coverage.

@Billy2011
Copy link
Contributor Author

you're re-downloading the same media initialization over and over for each segment that comes after the EXT-X-MAP tag, which is wasteful.

Also not sure what the purpose of the init_map and maps list is. Each Segment's map attribute gets set on all segments following the EXT-X-MAP, as defined by the HLS spec:

Maybe you missed my comments:

The map segment has to be put in the queue only once, and is then valid for all following segments until an ext_x_discontinuity tag arrives (in this case, a possibly new map segment follows) or the stream ends.

I want to save the init maps in a separate list and not in each Segment as before, because I want only the index of the init. map store, so less memory is used, especially with very long playlist of vod’s - do you have any objections, @beardypig, @bastimeyer?

@Billy2011
Copy link
Contributor Author

With this apple demo it comes to many errors in the output stream:

[mpegts @ 000002026bb82900] Non-monotonous DTS in output stream 0:1; previous: 1028520, current: 896040; changing to 1028521. This may result in incorrect timestamps in the output file.

@Billy2011
Copy link
Contributor Author

@bastimeyer,

A much better implementation would be fetching the media initialization once and writing it to the buffer in the HLSStreamWriter.write method before it writes the fetched segment data.

That was my thought at first, but I’ve been dealing with it for days and my current solution is already quite optimal and you will probably find that too, if you take it seriously. The handling of the byterange requires in the fetcher to create a sequence if you want to use the existing function for it, apart from the additional code this does not bring any advantage over my solution.

@bastimeyer
Copy link
Member

The map segment has to be put in the queue only once

Wrong. The media initialization data of the EXT-X-MAP tag needs to be written to the output buffer before every segment which comes after the tag. This has to be done until the playlist ends or until a discontinuity tag is set. By only putting the "map segment" with the media initialization to the writer once (yielding it in the worker's iter_segments()), you are corrupting all following segments in the output except the first one.

See here:
https://tools.ietf.org/html/rfc8216#section-4.3.2.5

The EXT-X-MAP tag specifies how to obtain the Media Initialization Section (Section 3) required to parse the applicable Media Segments. It applies to every Media Segment that appears after it in the Playlist until the next EXT-X-MAP tag or until the end of the Playlist.

The init_map attribute is not needed, as each segment already carries the map information set by the parser (the "map" state does not get popped in get_segment(), only read). The memory optimization by storing an index of all maps is also unneeded, as the map attribute on each segment only stores a reference to the instance of the Map namedtuple which carries the URL and byterange information.


Sorry, but I don't think that your proposed solution is optimal. You are abusing the current implementation of the Worker/Writer and how the Sequences/Segments are used to fetch data and how it's written to the output buffer. Map data needs to be fetched once, cached and then properly applied to the output buffer depending on each Segment's map attribute value. A proper implementation requires a bit, maybe even lots of refactoring of the methods of the {HLS,Segmented}{Writer,Worker} classes and maybe also the HLS parser, because there's currently no caching logic built in. Not to mention timing issues between the threads.

Adding hack after hack without any tests will make this an unmaintainable and inextensible mess.

@Billy2011
Copy link
Contributor Author

The media initialization data of the EXT-X-MAP tag needs to be written to the output buffer before every segment which comes after the tag. This has to be done until the playlist ends or until a discontinuity tag is set.

If that were the case, all my swiss wilmaa links would not run.

The memory optimization by storing an index of all maps is also unneeded, as the map attribute on each segment only stores a reference to the instance of the Map namedtuple which carries the URL and byterange information.

You are partly right, but an int uses significantly less memory than a namedtuple.

Adding hack after hack without any tests will make this an unmaintainable and inextensible mess.

I think the best tests are real life tests. The python tests only check the rudimentary functions, but that does not mean that those tests are useless.

@bastimeyer
Copy link
Member

Well, in case of MPEG-TS segments, section 3.2 of the RFC 8216 says this:

The Media Initialization Section of an MPEG-2 Transport Stream Segment is a Program Association Table (PAT) followed by a Program Map Table (PMT).
Each Transport Stream Segment MUST contain a PAT and a PMT, or have an EXT-X-MAP tag (Section 4.3.2.5) applied to it.

Which means that when an EXT-X-MAP tag is present with MIS data, it has to be applied to all segments, not just the first one.

For fMP4 segments in section 3.3, it's more complicated. The MIS via EXT-X-MAP is a requirement (hence your PR for resolving #3328), but since fMP4 is not a transport stream and since the paragraphs don't make it perfectly clear, I am not sure if applying the MIS to all segments is a necessity here. @beardypig, do you know more? Also, what happens to start offsets or the live-edge value here?

an int uses significantly less memory than a namedtuple

The same instance of the Map namedtuple (stored on the state dict) is shared between all Segments in their map attribute. As I've already said and linked, it does not get removed from the state when a segment gets created, unlike other attributes on the state.

I think the best tests are real life tests.

No. Unit and integration tests ensure that the implementation is correct and that future changes to any part of the project won't break anything.

@gravyboat
Copy link
Member

No response in ~3 months. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modern fmp4 HLS Incompatibility

4 participants