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

Do not add ID3 stream if output_id3_timestamps is disabled #1117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeoliva
Copy link

@jeoliva jeoliva commented Apr 11, 2020

Currently, no matter if vod_hls_output_id3_timestamps is enabled or not, the ID3 stream is always created in mpeg-ts segments of hls based outputs.

Given I have not seen any other use of the ID3 stream, but when vod_hls_output_id3_timestamps is enabled, and that I have found playback compatibility issues with older devices (smarttvs) when they find ID3 based streams in ts segments, I would like to propose create the ID3 stream just whenever it is going to be used.

@kaltura-hooks
Copy link

Hi @jeoliva,
Thank you for contributing this pull request!
Please sign the Kaltura CLA so we can review and merge your contribution.
Learn more at http://bit.ly/KalturaContrib

@erankor
Copy link
Contributor

erankor commented Apr 13, 2020

Thank you for your patch.
This behavior was implemented intentionally - when we started this project, we migrated from another solution. That's how it was there, and we wanted to keep it to reduce risk.
To date, we haven't had any issues because of it, and therefore, I don't want to take this risk now.

@jeoliva
Copy link
Author

jeoliva commented Apr 18, 2020

In case it helps to you, or anyone having this same problem, we detected this problem on WebOS 2.0 based devices. They are not able to manage ID3 type of tracks/info. Even we needed to modify mpegts program descriptors (

0x25, 0x0f, 0xff, 0xff, 0x49, 0x44, 0x33, 0x20, 0xff, 0x49,
) to don't include ID3 info.

Working like a charm after those changes.

If this is useful to you, I will push the program descriptor based changes.

@erankor
Copy link
Contributor

erankor commented Apr 18, 2020

If it's controlled by a conf param, I think we can merge it.
If you want to implement it -

  1. I think it makes sense to add a new struct 'mpegts_encoder_conf_t' that will hold interleave_frames, align_frames and the new flag.
  2. Please avoid pushing the trailing-space changes in the same pull. I didn't pay much attention to this in this project, maybe I'll find/replace it in all files... But either way, it will be in a dedicated pull without any other change.

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

Successfully merging this pull request may close these issues.

3 participants