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

Dynamic HDR SEI messages in h265 bitstreams do not reach media Codec when played from .ts #7113

Closed
dbPhilips opened this issue Mar 19, 2020 · 5 comments
Assignees
Labels

Comments

@dbPhilips
Copy link

[REQUIRED] Issue description

In the 2.11.1 version and all tested previous versions, Dynamic HDR SEI messages in an h265 bitstream (such as defined in e.g. ETSI TS 103 433)
do not all reach the MediaCodec properly when the bitstream is packed in a .ts container. This poses a problem for streams that intend to
indicate video decoder meta data through SEI such as dynamic HDR systems.

It seems related to the current H265Reader.java implementation.

Since all of these dynamic HDR streams I have encountered do use AUD's, the simplest modification seems be to use these when available in a stream
in the sample reader. Will open a pull request to share this modification.

It is also observed that without the AUD modification some of the .ts content does not play smoothly. On Qualcomm SD855-based devices this also
triggers mediacodec errors logged in the logcat at the time of the hiccup.

"03-16 16:37:22.195 947 21162 E OMX-VDEC-1080P: Unable to convey fps info to driver, performance might be affected"

Unfortunately the content with the the hiccup is not content that we own or are allowed to distribute.

[REQUIRED] Reproduction steps

Play Hevc Dynamic HDR .ts video with exoplayer

[REQUIRED] Link to test content

The content was mailed to dev.exoplayer@gmail.com

[REQUIRED] A full bug report captured from the device

No applicable for the given content

[REQUIRED] Version of ExoPlayer being used

2.11.1 ,2.10.5 and 2.10.2

[REQUIRED] Device(s) and version(s) of Android being used

Samsung Tab S6 (Android 9)

@marcbaechinger
Copy link
Contributor

Thanks. I assign to Santiago who is looking after the pull request also.

@ojw28
Copy link
Contributor

ojw28 commented Apr 19, 2020

@dbPhilips - Please could you re-send the content. It's expired.

@ojw28 ojw28 assigned ojw28 and unassigned AquilesCanta Apr 19, 2020
@dbPhilips
Copy link
Author

Done!

New one is valid up to 25th of April. This the maximum our IT department unfortunately allows.

ojw28 added a commit that referenced this issue Apr 20, 2020
This change generalizes the concept of "reading parameter sets" to
"reading prefix NAL units", ahead of a change that will treat AUD
and suffix SEI NAL units in the same way.

The change also introduces some static isXxxNalUnit methods for
clarity.

Issue: #7113
PiperOrigin-RevId: 307376967
ojw28 added a commit that referenced this issue Apr 20, 2020
@ojw28
Copy link
Contributor

ojw28 commented Apr 20, 2020

Thanks!

The proposed fix in #7115 didn't seem ideal to me. It relied on the presence of AUDs to avoid dropping the prefix SEI NAL units, however AUDs are optional and it should be possible to do the right thing even if they're omitted.

I've merged the two changes ref'd above as an alternative solution. These changes should be able to handle prefix SEI NAL units when AUDs are omitted as well. It also fixes how suffix SEI NAL units are handled, to make sure that they're output in the correct sample as well. Looking at the NAL unit types output in each sample for the test content, it appears the changes do the right thing. Before the pattern looked like this, where each line shows the NAL unit types in a single sample:

NAL types: 32, 33, 34, 39, 39, 39, 39, 21
NAL types: 9
NAL types: 9
NAL types: 1
NAL types: 1
NAL types: 32, 33, 34, 39, 39, 39, 39, 21
NAL types: 9
NAL types: 9
NAL types: 1
NAL types: 1
...

After it looks like this. Note we now include the AUD (35) at the start of each sample, and the prefix SEI NAL units (39). The latter would be included even if the AUDs are omitted in the stream:

NAL types: 35, 32, 33, 34, 39, 39, 39, 39, 21
NAL types: 35, 39, 39, 9
NAL types: 35, 39, 39, 9
NAL types: 35, 39, 39, 1
NAL types: 35, 39, 39, 1
NAL types: 35, 32, 33, 34, 39, 39, 39, 39, 21
NAL types: 35, 39, 39, 9
NAL types: 35, 39, 39, 9
NAL types: 35, 39, 39, 1
NAL types: 35, 39, 39, 1
...

I wasn't able to test user observable behaviour, so please do check out dev-v2 and confirm on your side that the issue really is fixed. Thanks!

@ojw28 ojw28 closed this as completed Apr 20, 2020
@dbPhilips
Copy link
Author

Can confirm the issues on our side also get fixed with these commits.

The alternative solution indeed covers more cases so its a better fix. Great work!

Thanks!

ojw28 added a commit that referenced this issue May 28, 2020
This change generalizes the concept of "reading parameter sets" to
"reading prefix NAL units", ahead of a change that will treat AUD
and suffix SEI NAL units in the same way.

The change also introduces some static isXxxNalUnit methods for
clarity.

Issue: #7113
PiperOrigin-RevId: 307376967
ojw28 added a commit that referenced this issue May 28, 2020
@google google locked and limited conversation to collaborators Jun 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants