Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdded id property to the MediaFormat class #884
Conversation
Added the property 'id' to the MediaFormat class which serves as an identifier for the track. DASH Representations will have the "id's" from their Media Presentation Description mapped to the id property in the MediaFormat class that will represent the track. We needed this for an use case where we wanted to read the 'id' value from the DASH representation and present it to the user in order for the user to select the right track.
…d-dash Syncing my fork
This comment has been minimized.
This comment has been minimized.
In some way this is similar to the trackId field add in decb7f5 although you are exposing DASH representation id instead of a numeric identifier. Out of curiosity, what is the use case behind showing the DASH representation id directly to the user? Are you "hand-making" this id's to contain information (resolution, bitrate, etc) that could be useful for the user? |
This comment has been minimized.
This comment has been minimized.
We provide the users of our product the ability to load any DASH stream on the player, and in many cases users load DASH streams with multiple video representations (tracks). We saw a case where a user would load a stream with various video representations that had the same height and width but a different bitrate. In our UI there was no way to differentiate the streams from each other, except for selecting them on the player and trying to figure out the visual difference. With this change users can specify how the tracks in our UI should be displayed by setting the I also noticed that |
This comment has been minimized.
This comment has been minimized.
I created a screenshot using the ExoPlayer demo app to demonstrate the functionality. DASH MPD:
|
This comment has been minimized.
This comment has been minimized.
Ok, thank you very much for the detailed explanation. |
Hey. Thanks for the contribution. I'm a little bit confused by this comment: We saw a case where a user would load a stream with various video representations that had the same height and width but a different bitrate. In our UI there was no way to differentiate the streams from each other. It's unclear why you need the identifier to resolve this issue. The ExoPlayer.getTrackFormat method gives you the MediaFormat of the track, which includes the bitrate. So why not just look at the bitrate field to solve this case? The above question aside, I do think it's a good idea to propagate the identifier, and can see cases where it would be useful. We'll try and get this merged soon, or an equivalent change in place. One thing we're mulling over is whether a separate field is the best option, or whether we should just convert the existing field to a string. I think we're thinking that converting the existing field is probably the best option. |
/** | ||
* An identifier for the format. | ||
*/ | ||
public final String id; |
ojw28
Oct 23, 2015
Contributor
As per comment, it seems better just to convert trackId into a string?
As per comment, it seems better just to convert trackId into a string?
rotationDegrees, pixelWidthHeightRatio, channelCount, sampleRate, language, | ||
subsampleOffsetUs, initializationData, adaptive, maxWidth, maxHeight); | ||
} | ||
|
||
public MediaFormat copyAsAdaptive() { | ||
return new MediaFormat(trackId, mimeType, NO_VALUE, NO_VALUE, durationUs, NO_VALUE, NO_VALUE, | ||
return new MediaFormat(id, trackId, mimeType, NO_VALUE, NO_VALUE, durationUs, NO_VALUE, NO_VALUE, |
ojw28
Oct 23, 2015
Contributor
This is problematic, because you're creating an adaptive MediaFormat (i.e. one that really spans multiple individual formats) from a non-adaptive one (i.e. an arbitrary one of the individual formats), and the adaptive MediaFormat is getting the id from the arbitrary individual format that you use.
You should probably take a look at the places that call this method, and either decide to set the id to null here or allow a different id to be passed in as an argument, so you can set it to some other value, depending on what you think makes most sense.
This is problematic, because you're creating an adaptive MediaFormat (i.e. one that really spans multiple individual formats) from a non-adaptive one (i.e. an arbitrary one of the individual formats), and the adaptive MediaFormat is getting the id from the arbitrary individual format that you use.
You should probably take a look at the places that call this method, and either decide to set the id to null here or allow a different id to be passed in as an argument, so you can set it to some other value, depending on what you think makes most sense.
Looks pretty good. If you convert the existing id field to a string and fix the id when creating adaptive formats to do something sensible, then we'll merge this. Thanks! |
We would like to give users the possibility to use a 'custom' name for their tracks instead of using a predfined format. We figured out we could make this possible by using the I will convert the existing field and fix the id when creating adaptive formats. By the way: I was just told we had some technical problems signing the corporate CLA, one of my supervisors was not able to sign it through the webpage. He contacted Google and he's working on it. |
@@ -45,20 +45,20 @@ public void testConversionToFrameworkFormat() { | |||
initData.add(initData2); | |||
|
|||
testConversionToFrameworkFormatV16(MediaFormat.createVideoFormat( | |||
MediaFormat.NO_VALUE, "video/xyz", 5000, 102400, 1000L, 1280, 720, initData)); | |||
null, MediaFormat.NO_VALUE, "video/xyz", 5000, 102400, 1000L, 1280, 720, initData)); |
ojw28
Oct 26, 2015
Contributor
Please put the indentation back to what it used to be like (4 chars) (ditto x2 below).
Please put the indentation back to what it used to be like (4 chars) (ditto x2 below).
@@ -38,12 +38,11 @@ | |||
* the timestamps of their parent samples. | |||
*/ | |||
public static final long OFFSET_SAMPLE_RELATIVE = Long.MAX_VALUE; | |||
|
ojw28
Oct 26, 2015
Contributor
Add blank line back again?
Add blank line back again?
@@ -846,6 +846,7 @@ public void run() { | |||
|
|||
public ExposedTrack(MediaFormat trackFormat, int adaptationSetIndex, Format fixedFormat) { | |||
this.trackFormat = trackFormat; | |||
|
ojw28
Oct 26, 2015
Contributor
Rm additional blank line.
Rm additional blank line.
|
||
} | ||
} |
ojw28
Oct 26, 2015
Contributor
Revert bad style change.
Revert bad style change.
Happy to merge this once stylistic alterations are reverted. I don't think the CLA has been signed yet. Any update? Thanks! |
I have no idea why Android Studio screwed up the code identation, it should detect existing file identation and use that for editing. The person responsible for signing the CLA is out of office today. I'll try to update you on the CLA tomorrow. Thanks. |
CLA looks good now. Feel free to resolve the merge conflict and update if you have a chance. Otherwise I can probably do this myself tomorrow. Thanks! |
In our application we give people control of the "selected" track on the player for DASH streams however there was no way for people to identify the track, unless if you knew the bitrate or width/height properties. This change adds the
id
property to the MediaFormat class which is read from the DASH Media Presentation Description. This way it's easier for users to identify tracks on the player.CLA should be signed: https://groups.google.com/forum/#!forum/jwplayer-google-contributors
Feedback is appreciated, feel free to ask questions.
Thanks.