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

User hooks for hls parser #1998

Closed

Conversation

anuragkalia
Copy link

This is regarding the issue #1983.

I have added an extra function onSegmentParsed on the playerInterface passed to each manifest parser. If the manifest parser wants, it can call the function with mime type, stream id, and data (whose format may differ according to the manifest parser) whenever it encounters a new fragment in its stream.

This function right now simply emits an event 'segmentparsed' from the player when this function is called.

Also find the unit tests to go along with it. And let me know if there is any improvement to be made. :)

Anurag Kalia added 2 commits June 12, 2019 17:55
Add `onSegmentParsed` to playerInterface for manifest parsers. This
callback will be triggered to communicate custom metadata of the segment
when it is being parsed in the manifest, and can be read on using
the event 'segmentparsed'. Right now, only HLS is using this to
communicate tags associated with each segment.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/player.js Show resolved Hide resolved
1. Refactor code+comments to make changes clearer
2. Document SegmentParsedEvent
3. Modify the format of SegmentParsedEvent
Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

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

Thanks for the documentation! It's a real improvement.
I just have a few more requests.

lib/player.js Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
lib/hls/hls_parser.js Outdated Show resolved Hide resolved
Removed the nested 'detail' property to match other events.
@anuragkalia
Copy link
Author

I really appreciate the quick feedback. Have added jsdoc. Let me know if there is anything else that needs to be done.

In particular, should I add a test case for checking for the case when 'segmentparsed' is fired?

Aside: About the pull request process, I add a comment for each change you request and click 'Resolve'. Is that the correct process? How do you get notified on your end when I push a commit?

@anuragkalia
Copy link
Author

I signed it!

@anuragkalia
Copy link
Author

Hi, is there any update on this? I am also confused why googlebot is failing the CLA check.

@anuragkalia
Copy link
Author

I signed it!

@TheModMaker
Copy link
Contributor

Hi, is there any update on this? I am also confused why googlebot is failing the CLA check.

You need to sign the CLA with the email you use to login to GitHub. It appears the email you used in the commits has signed the CLA, but the login email for GitHub doesn't. I think you can also add the email you signed with to your GitHub account as an alternative email and that should work too.

@anuragkalia
Copy link
Author

I have added the email Anurag.kalia@hotstar.com to github quite already. How much time does it take for Google it to check it again? Also, thanks a lot for taking your time out for this troubleshooting which has no relation to shaka-player. :)

@anuragkalia
Copy link
Author

I signed it!

* @property {number} streamId
* Id of the stream that segment belonged to.
* @property {*} segmentData
* Its format depends on the manifest parser.
Copy link
Member

Choose a reason for hiding this comment

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

This seems particularly problematic to me. For one, it's not obvious what the DASH equivalent is to the HLS type you're sending through here. For another, the HLS type you're sending through here is an internal object, the format of which we should be free to change and the compiler should be free to rename. So I wouldn't want to send that outside the Player to the app.

Copy link
Author

@anuragkalia anuragkalia Jun 21, 2019

Choose a reason for hiding this comment

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

I am still in the beginning of how DASH spec but, from my limited understanding, emsg is the equivalent in DASH. (I saw that emsg is handled separately in code right now). I would like to be corrected on this one. Moreover, given shaka does not intend to be restricted to HLS and DASH but intends to support any manifest, is it not good to provide a general purpose function for extracting custom data from the manifest?

I completely agree with the fact that right now we are sending an internal object directly. I did not account for the fact that users of the method could possibly mutate the object.

Thank you for your time and looking forward to your thoughts.

Copy link
Author

Choose a reason for hiding this comment

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

After thinking a little more about it, I realized that a user can always write their own parser if they have to read the custom data from the manifest. That exactly is something I wanted to avoid and thus came up with this PR.

Also, though emsg indicates segment specific details, it seems to be a feature of mp4 rather than DASH. Am I correct here?

Copy link
Member

Choose a reason for hiding this comment

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

The 'emsg' box is an MP4 box, but it came out of the DASH spec if I remember correctly. There is a schema specified in DASH that means "reload the manifest when you see this". Shaka Player handles that directly, and passes any other schema up to the application.

@@ -4249,6 +4269,26 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
}
}

/**
* Callback from player
Copy link
Member

Choose a reason for hiding this comment

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

This is a method on Player, so this comment can't be right.

@@ -555,6 +565,67 @@ describe('HlsParser live', () => {
master, media, [ref1], mediaWithAdditionalSegment, [ref1, ref2]);
});

it('triggers segment parsed callbacks as segments appear', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding tests!

@joeyparrish joeyparrish added the type: enhancement New feature or request label Sep 30, 2019
@vlee-harmonicinc
Copy link

vlee-harmonicinc commented Nov 19, 2020

emsg came out of the DASH Spec, but its concept could be applied to HLS.
HLS support emsg box in MP4 now. It is added to metadata textTrack as cue, just like how HLS process id3 in ts. Below code could extract emsg message, tested in Safari 13 and 14 with browser native player.

        // player = HTMLMediaElement with src= CMAF HLS with emsg
	player.textTracks.addEventListener('addtrack', (event) => {
		if (event.track.kind === 'metadata') {
			event.track.mode = 'hidden';
			event.track.addEventListener('cuechange', (event) => {
				for (let cue of event.target.activeCues){
					console.log(cue.value.key, cue.value.data);
				}
			});
		}
	});

It would be nice if Shaka player support emsg of HLS.

@joeyparrish
Copy link
Member

emsg came out of the DASH Spec, but its concept could be applied to HLS.
HLS support emsg box in MP4 now. It is added to metadata textTrack as cue, just like how HLS process id3 in ts. Below code could extract emsg message, tested in Safari 13 and 14 with browser native player.

...

It would be nice if Shaka player support emsg of HLS.

That sounds reasonable. Would you be willing to put together a PR to dispatch the appropriate Player events for that in src= mode? Thanks!

@vlee-harmonicinc
Copy link

vlee-harmonicinc commented Dec 10, 2020

Metadata event of src= mode has been already dispatched 6 months ago.
#2476

It seems that metadata in emsg (CMAF) + HLS + MSE mode has not been supported yet. streaming_engine ignores emsg if emsgSchemeIdUris does not exists.

@avelad
Copy link
Member

avelad commented Jun 3, 2022

@anuragkalia I see that there are conflicts to be able to merge, can you rebase?

@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jun 3, 2022
@avelad
Copy link
Member

avelad commented Jun 13, 2022

Closing due to inactivity.

@avelad avelad closed this Jun 13, 2022
@NoChance777
Copy link
Contributor

NoChance777 commented Jun 13, 2022

Hello guys, I've been tracking this ticket for a while already. We need this feature for being able to extract scte35 events from a HLS stream.

@avelad
Copy link
Member

avelad commented Jun 13, 2022

This is an abandoned PR, right now we are not working on this functionality, if you are interested you can open a PR and we will review it as soon as possible. Thanks!

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
@github-actions github-actions bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 25, 2023
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: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants