-
Notifications
You must be signed in to change notification settings - Fork 6k
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
FLV (H.264 + AAC) support #828
Conversation
In general I'm not a fan of adding support for legacy container formats. If we take it, it's something we have to commit to maintaining. However, as you say, this does look pretty simple, and so it seems like a reasonable request! I'll post a few initial comments now, and try and have a more in-depth look later this week. Thanks! |
@@ -146,6 +146,13 @@ public UnrecognizedInputFormatException(Extractor[] extractors) { | |||
} catch (ClassNotFoundException e) { | |||
// Extractor not found. | |||
} | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the list in the javadoc at the top of this class to say that FLV is supported. Also, if seeking is not supported (see comment below) then update the sentence that says seeking in AAC and MPEG-TS is not supported, to include FLV too.
Taking a look to all these things. |
Just incorporated suggested changes. Thanks @ojw28 for all your comments! |
Sorry for the delay looking at this. Will try and get to it tomorrow! |
@@ -60,7 +60,7 @@ | |||
* <li>MPEG TS ({@link com.google.android.exoplayer.extractor.ts.TsExtractor}</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list should contain FLV.
Changes applied |
FLV is useful in live streaming with low-latency, similar to RTMP for flash. |
@jeoliva Thanks for your work, I merged this pull request to my fork, 👍 |
Pair<Integer, Integer> audioParams = CodecSpecificDataUtil.parseAacAudioSpecificConfig( | ||
audioSpecificConfig); | ||
|
||
MediaFormat mediaFormat = MediaFormat.createAudioFormat(MimeTypes.AUDIO_AAC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compile failed when merge to dev-1.5.1-rc:
git/ExoPlayer/library/src/main/java/com/google/android/exoplayer/extractor/flv/AudioTagPayloadReader.java
Error:(111, 44) error: method createAudioFormat in class MediaFormat cannot be applied to given types;
required: int,String,int,int,long,int,int,List<byte[]>,String
found: String,int,int,long,Integer,Integer,List<byte[]>,
reason: actual and formal argument lists differ in length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems change to the bellow is ok:
MediaFormat.createAudioFormat(MediaFormat.NO_VALUE, MimeTypes.AUDIO_AAC,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fix it at: winlinvip@115aee8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I've merged this and fixed the break. Thanks @jeoliva / @winlinvip !
I've also submitted some stylistic tweaks just to bring the code in line with the rest of the source: 950cc70 |
And some further changes. I was able to simplify ScriptTagPayloadReader somewhat. Please take a look, and let me know if I accidentally broke anything! |
One final question: Where did the test link you've added come from? I want to know if it's fine to copy it across to our own hosting. Obviously the underlying content (big buck bunny) is fine for us to copy. Depending on who packaged it, we might need to re-package it ourselves to copy it over. Please advise. Thanks! |
I test the flv, seems ok for vod stream, for example, it's ok for http://www.ossrs.net/mp4/source.200kbps.768x320.flv。 |
Thanks. for the info. I made some further changes to the extractor in 4422e8a based on an internal review. I've put a couple of comments in the old version of the code in that change that explain why the new state "skipping to next tag header" state is necessary :). |
@ojw28 Thanks~ I will try to add hasVideo and hasAudio in header in SRS server. |
Fixed in ossrs/srs@50a7b9c, set hasVideo and hasAudio tag in flv header. |
But live time is not moving |
@chodison Not understand you, what's |
I see, you means the timeline of ExoPlayer, that's caused by the flv onMetaData packet:
Because the flv stream is generated by ffmpeg to publish a flv file, which contains the duration, I will remove this field for live stream. |
The player‘s position is always zero. And Cycle two times, it don't work. |
Fixed in ossrs/srs@ef00005, remove the duration of flv for live streaming. |
@ojw28 For flv vod video, can not ExoPlayer to support seeking? |
See @jeoliva's note on this thread above seeking: In fact, FLV container doesn't include any kind of information to help on implementing seeking. If you want your media to be seekable you should use a more appropriate container format, like MP4. |
@ojw28 @jeoliva @chodison There is some workaround for flv to support seek, the service which produce the flv file must inject all keyframe offset to the onMetaData script tag, then player can got the offset to seek to specified time. However, it's not the standard of flv, it's possible to implements by this workaround. |
@ojw28 why the ffplay player can seek flv vod video? |
@chodison The ffplay generally play a local flv vod file, it can skip one tag by tag, it's ok for disk but not good for http flv vod stream. The network file always need a |
@winlinvip My tested url is http flv vod stream. |
@chodison Http flv vod stream is ok to seek, to skip one tag by tag, it's not efficient way however. The acceptable way for a big flv vod file to seek, is inject the keyframe offset to the onMetaData. |
There's an alternative "accepted" way where the server will allow you to specify the seek position as, for example, a query parameter. In that case the server does the brute force tag-by-tag search (it's probably implemented to have built an index already) and serve you the media from the specified seek position. The takeaway here, however is that (a) FLV does not support seek properly, (b) we don't intend to support any hacks to make seeking in FLV possible in ExoPlayer, unless they're external contributions and do not extend anywhere outside of the FLV extractor package, and (c) if you're knowingly packaging your own media as FLV, you should stop and choose a more appropriate container like MP4. |
@ojw28 Aggree to use MP4 instead for http vod stream. 😄 |
@winlinvip @ojw28 flv vod stream server supports http start or http range to seek. |
@chodison Yep, but need the player to finger it out where to seek, for example:
The player must conver the
Or request by range whatever. The question is: how to know the offset and seconds relationship? The flv container never describe it. There is a workaround, to save the offset-seconds map to the I aggree, should never use none-standard, especially the player. |
@winlinvip http://server/f.flv?start=time or request by range, file offset is from he proportion of the position to the duration. |
@chodison seekTime/flvDuration != position/flvFileLength. |
The offset and time relationships are often sent in the metadata response; depending upon your server's implementation of course. For information on this, look here: http://h264.code-shop.com/trac/wiki/FlashPlayer |
We have no plans to support seeking in FLV. All of the approaches mentioned would add complexity to the upstream package, and would not work in a consistent way across different |
@winlinvip Estimation calculation. The flv vod stream server will return the key frame for closest position |
FLV is not one of the most used containers right now, and of course not a priority for any player, but I though it could be useful adding it to Exoplayer for 2 reasons:
Regarding the implementation, as title stated, this pull request is adding support for FLV container, and more specifically, when content within FLV is encoded using H.264 and AAC.