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

alternatives does not contain audio playlist after upgrading from 3.0.0 to 5.0.3 #62

Open
bes opened this issue Dec 15, 2022 · 16 comments
Assignees

Comments

@bes
Copy link

bes commented Dec 15, 2022

Hi, I recently upgraded from 3.0.0 to 5.0.3 and now my audio playlist ends up in unknown_tags instead of in alternatives.

What am I doing wrong?

Here is the master playlist, slightly anonymized

#EXTM3U
#EXT-X-VERSION:6
#EXT-X-INDEPENDENT-SEGMENTS
#EXT-X-MEDIA:LANGUAGE="en",AUTOSELECT=YES,FORCED=NO,TYPE=AUDIO,URI="https://xyz.com/a/playlist.m3u8",GROUP-ID="audio",DEFAULT=YES,NAME="Audio"
#EXT-X-STREAM-INF:RESOLUTION=3840x2160,FRAME-RATE=25.0,BANDWIDTH=6878544,AUDIO="audio"
https://xyz.com/b/playlist.m3u8
@sdroege
Copy link
Collaborator

sdroege commented Dec 16, 2022

The FORCED attribute is only allowed in TYPE=SUBTITLES alternatives, that's why parsing it fails.

 FORCED

 The value is an enumerated-string; valid strings are YES and NO.
 This attribute is OPTIONAL.  Its absence indicates an implicit
 value of NO.  The FORCED attribute MUST NOT be present unless the
 TYPE is SUBTITLES.

@sdroege sdroege closed this as completed Dec 16, 2022
@bes
Copy link
Author

bes commented Dec 16, 2022

Thank you for your help.

Is there no room for lenient parsing? What if new attributes are added to the HLS spec, will those playlists be rejected too, even if they could be played?

I would be happy if m3u8-rs could follow the robustness principle - be conservative in what you send, be liberal in what you accept.

@sdroege
Copy link
Collaborator

sdroege commented Dec 16, 2022

New attributes would be stored separately IIRC, that wouldn't be a problem.

The spec is very clear about this part (MUST NOT), that's why this is currently considered an actual parsing error. Maybe @vagetman or @rutgersc have other opinions here?

@sdroege sdroege reopened this Dec 16, 2022
@bes
Copy link
Author

bes commented Jan 9, 2023

Happy new year! Any thoughts on this issue @vagetman @rutgersc ? Thanks 🙏

@bes
Copy link
Author

bes commented Jan 19, 2023

I am stuck on v3.0.0 until this issue is resolved. Unfortunately I just discovered that v3.0.0 has a bug that makes it hang forever if the m3u8_rs::parse_playlist function gets an empty slice.

Since this issue isn't making any progress, and since you're not patching v3.0.0 I am wondering if anyone can give me advice on what I should do with "bad" playlists that contain the FORCED attribute (or any other attribute) where it shouldn't be?

Should I do a first pass over the playlist and remove things myself? Seems brittle and error-prone :(

(Unfortunately my software receives bad playlists, and there's not much I can do about that)

EDIT: v4.0.0 also works for me, and doesn't have the hanging bug.

@sdroege
Copy link
Collaborator

sdroege commented Jan 20, 2023

@vagetman @rutgersc We could also add a "strict" flag to the parser and only when that is set to true it would fail parsing such things, otherwise just ignore as much as possible.

@rutgersc
Copy link
Owner

Yeah, a parameter like that would probably be the way to go. I'm honestly kind of surprised the spec is this strict about some property belonging to a specific type.

@sdroege
Copy link
Collaborator

sdroege commented Jan 23, 2023

Ok, let's go with that then. I'll look into adding this some time this week.

I'm honestly kind of surprised the spec is this strict

... and on the other hand the HLS spec is extremely loose about things that actually matter. It's one of the worst specs I ever worked with.

@sdroege sdroege self-assigned this Jan 23, 2023
@vagetman
Copy link
Contributor

Sorry, I was a bit busy. @bes, to clarify - what would be the desired behavior? Did you want to ignore FORCE on the wrong alternatives during ingestion?

@bes
Copy link
Author

bes commented Feb 10, 2023

@vagetman Hi! My thinking is that there are a lot of playlists out there that are plain wrong and it might be any attribute, not just FORCED.

My dream would be if m3u8-rs could parse as liniently as possible, for any attributes and any items, not just limited to FORCED.

But when it came to producing playlists itself, it would be according to spec without the possibility to introduce errors.

But how far you take it is obviously your decision.

Basically as I wrote in a message above: follow the robustness principle - be conservative in what you do, be liberal in what you accept from others

Thank you 🙏

@sdroege
Copy link
Collaborator

sdroege commented Feb 10, 2023

@vagetman Please feel free to take over here. I'm extremely busy with work-related tasks currently and this keeps getting behind on my todo list. Sorry for that.

parse as liniently as possible

FWIW this is exactly the reason why we're in this situation now. Because the parsers are very forgiving, most of the HLS streams are complete garbage and hard to handle. It's the same with various other formats.

At this point it's the only valid approach for HLS though, that ship has sailed. And additional attributes that have no valid meaning is the least of the problems in that regard :)

@vagetman
Copy link
Contributor

vagetman commented Feb 10, 2023

Thanks, both. I'll poke around. I'm busy with Super Bowl prep and delivery this week and the weekend, but the following week should be easier.

I have a different experience with playlists though. I actually didn't see any HLS deviations around here.. But that could be because I'm usually working with big broadcasters and streamers and their playlists come from places like Elementals or Wowza. Further, customers normally want to validate their streams with Safari and QuickTime and those are not exactly forgiving ones. They simply stop playing if something is wrong without much of verbosity. I'm not sure what happens with extra attributes, they could be ignored.

Also, generally, because something is working despite the standard it's not a good example to follow. BUT I was advocating on another ticket we should support unknown attributes to support experiments or future standard revisions without having a necessity to update the library.

@vagetman
Copy link
Contributor

@bes so, where does it actually break for you? I tried ingesting the manifest with this and it reads and prints it just fine:

    let playlist = match parsed {
        Result::Ok((_i, playlist)) => playlist,
        Result::Err(e) => panic!("Parsing error: \n{}", e),
    };

    match playlist {
        Playlist::MasterPlaylist(pl) => println!("Master playlist:\n{:?}", pl),
        Playlist::MediaPlaylist(pl) => println!("Media playlist:\n{:?}", pl),
    }

I can see some checks on FORCED in AlternativeMedia::from_hashmap(). Is this a problem? I thought the claim is that manifest parsing doesn't work.

@bes
Copy link
Author

bes commented Feb 12, 2023

My original claim is that an audio playlist that has FORCED=NO on it ends up in unknown_tags which makes it much harder to work with.

@vagetman
Copy link
Contributor

Ah, thank you @bes

@bes
Copy link
Author

bes commented Jul 13, 2023

Hello everyone, I was wondering if any progress was made on this issue? Thanks 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants