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

ffmpeg demuxer: faster channel change for PVR addons ... #3590

Closed
wants to merge 3 commits into from

Conversation

margro
Copy link
Contributor

@margro margro commented Nov 7, 2013

without internal demuxing

(such as MediaPortal, ArgusTV, MythTV, NextPVR)

This pull request contains a Gotham port of the original work from @FernetMenta with a few changes from @davilla
(FernetMenta@bf753eb

It is a follow-up for #1757

FernetMenta asked me to port and test his work because he couldn't test it as a VDR user.

// Set streaminfo false and skip avformat_find_stream_info (slow)
// But we need a proper codec extradata so fill it in for ffmpeg.
// This routine is taken from avformat_find_stream_info and friends.
if(st->parser && st->parser->parser->split && !st->codec->extradata)

This comment was marked as spam.

@popcornmix
Copy link
Member

I'd like to encourage this patch. It's been widely used in test builds on Pi with positive reports.
I'd also like this patch which enable fast start for all files (not just pvr) to be considered:
popcornmix@cda4e3c
For example it improves startup time for a .ts file from 35 seconds to 1.5 seconds:
http://forum.xbmc.org/showthread.php?tid=176687&pid=1542692#pid1542692
(although I am aware of an mp4 file with no audio that this patch breaks)

@whaupt
Copy link
Contributor

whaupt commented Nov 8, 2013

It breaks http streaming for me too.
Maybe anyone can verify this, because it's interesting that there seems to be a relation.

@margro
Copy link
Contributor Author

margro commented Nov 8, 2013

@FernetMenta : I will add popcornmix@cda4e3c
and update the comments with your remarks

@popcornmix: Right, now I don't want to enable it for other purposes than Live TV, because this will also break quite some video addons (e.g. ITVPlayer, Vevo). I would like to stay on the safe side for Gotham right now...

@whaupt : You mean popcornmix' addition or my original patch?
Is this a public http stream which I could test?

@popcornmix
Copy link
Member

@margro
Sure, it shouldn't be enabled if it breaks things. But understanding why it breaks things sounds useful, just to be sure those reasons don't apply to live tv.

@FernetMenta
Copy link
Contributor

@popcornmix can you provide this mp4 file not working?

@Uukrull
Copy link
Contributor

Uukrull commented Nov 8, 2013

I tested popcornmix changes with the DVBViewer PVR (it uses http streams) and it's working fine. Channel changes seem to be slightly faster.

@whaupt
Copy link
Contributor

whaupt commented Nov 8, 2013

@margro I applied FernetMentas version on 12.2 and if I set streaminfo boolean to false for every protocol, it breaks http streaming.
at-visions@a26c2c0

maybe my version reacts slightly different to gotham, just wanted to share my findings.
for testing I just placed any video file on a local webserver. stream was buffering forever.

@popcornmix
Copy link
Member

@FernetMenta I've PM-ed you the mp4 file.

@FernetMenta
Copy link
Contributor

the various formats behave slightly different. for mpeg-ts ffmpeg creates streams according to pat/pmt. when it creates a stream, it sets the need_parsing flag.

I did a quick test with the provided mp4 file. there's no parser created for the video stream. changing the code to this made it work.

      if(!st->parser)
      {
        st->need_parsing = AVSTREAM_PARSE_FULL;
      }

      if(st->parser && st->parser->parser->split && !st->codec->extradata)

note, this was only a quick test. i think this can be done better.

@popcornmix
Copy link
Member

I can confirm @FernetMenta latest patch fixes the mp4 file.
One minor issue - you get to see the visualisation start up (just for a second) when playing a file with audio and video - presumably because the audio stream is detected first.

@FernetMenta
Copy link
Contributor

I sent some refactoring to @margro FernetMenta@c425470

I summarize:

  • avformat_find_stream_info is slow
  • for mpeg-ts we detect program changes. after such a change we need a method to get stream information anyway
  • avformat_find_stream_info rewinds to pos 0 after analysis which IMO is a bad idea for live streams
  • maybe add an advanced setting for skipping streaminfo in general?

@elupus what's your opinion on this?

@popcornmix
Copy link
Member

For the file playback case:
Is assume avformat_find_stream_info is only slow for non-indexed formats (mpeg-ts/mpeg-ps).
I assume it is fast for indexed formats (mkv/avi).

I believe it avformat_find_stream_info is not just slow for non-indexed files, it is inaccurate (an arbitrary amount of time is parsed, and is a stream is not present during that time it is missed).

Would skipping avformat_find_stream_info for non-indexed formats, but using it for index formats be reasonable?

@FernetMenta
Copy link
Contributor

Or maybe we should limit the time for analysis in general.

@ghost ghost assigned elupus Nov 10, 2013
@margro
Copy link
Contributor Author

margro commented Nov 10, 2013

@FernetMenta: Added and tested your changes. Will squash them later.
Limiting the analysis time (e.g. only for Live-TV) will work also. That was what I've used before.
The problem is only that late appearing streams are missing.

@davilla
Copy link
Contributor

davilla commented Nov 10, 2013

see Pivosgroup@4abd884
there's another commit there related to this.
also line 802 -> st->codec->extradata_size = 0; maybe also return a null pkg here. The idea is to eat demux pkgs until we get valid extradata info. Then we can trust the info and have DVDPlayer open a codec.

setting AVDISCARD_ALL might also only be valid for mpeg2/h264. It really depends on what the FFMpeg codec does when discarding. Generally you want to skip the actual decode but allow the codec to parse the stream info.

@FernetMenta
Copy link
Contributor

I would not eat packets for a single stream. In case we know there is video and we are waiting for an IDR frame, we should eat all packets (audio as well). This is how I do it in vnsi.

@davilla
Copy link
Contributor

davilla commented Nov 10, 2013

yep

@elupus
Copy link
Contributor

elupus commented Nov 11, 2013

I really dislike this. Please fix ffmpeg :(.

@davilla
Copy link
Contributor

davilla commented Nov 12, 2013

Fixing ffmpeg would require major work I think, It's designed a very static situation and to try very hard to obtain correct info no matter the cost in start up time. Even decoding several frames which is a real hurt under ARM with HD content.

I've looked at what it does and I don't see a good/clean way to make a stream with known characteristics, startup faster. In general, we take 5 to 10 seconds (on ARM) after selection before there is any hint of a switch.

Maybe FFMpeg debs would have a clue on how to handle this.

@FernetMenta
Copy link
Contributor

The problem with ffmpeg is that the parser has no access to the decode extradata functions. fixing this and add a flag to ffmpeg to force continuous parsing of SPS would eliminate the need of instantiating a decoder here and also calling find_stream_info.
Not sure how difficult this would be.

@davilla
Copy link
Contributor

davilla commented Nov 12, 2013

You would still have to resolve FFMpeg doing full frame decodes when probing. This really hurts to do N-frames of 1080p on ARM. Each frame can take several hundred milliseconds to decode.

@whaupt
Copy link
Contributor

whaupt commented Nov 12, 2013

Does anyone know why FFMpeg even needs to decode instead of parse?
Another fact I can't explain is why FFMpeg uses much time looking into streams other than video/audio.

@FernetMenta
Copy link
Contributor

I did some more investigation. When reading packets through the format context, ffmpeg actually parses the packets. The problem is that it does not expose the desired info. The codec context is only updated when a frame is decoded.

@FernetMenta
Copy link
Contributor

really dislike this. Please fix ffmpeg :(.

@elupus this argument is really weak. this may not be the perfect solution but it is much better than we have now. thousands of users do suffer from this problem. I suggest that you either work with us on this or you don't block our work. we are short final for Gotham and we need a solution.

@elupus
Copy link
Contributor

elupus commented Nov 12, 2013

i'm not going to block it. but hate taking these types of approaches.

@margro
Copy link
Contributor Author

margro commented Nov 13, 2013

@FernetMenta: just tested (and added) your latest changes. Works fine for me. Although, I'm not sure about the global reduction of the max_analyze_duration to 0.5 s.
I've done this earlier also for my custom Frodo build and the result was that people started complaining about broken video addons (no audio, no video). I'm not sure if the extradata route will fix these previous "missing stream" issues.
So, probably we should make this value configurable in the advancedsettings of we should limit it to DVD+Live TV only.

@hudokkow
Copy link
Member

@FernetMenta

Can you point me to some of your test files or streams? I'm investigating a similar problem and would like to compare my XBMC and ff(av)play outputs with yours.

@davilla
Copy link
Contributor

davilla commented Nov 14, 2013

please don't limit this to 'DVD+Live TV only.' :) When one is switching between one of several udp transport streams in a playlist, it's not a DVD nor it is LiveTV.

@elupus
Copy link
Contributor

elupus commented Dec 13, 2013

Here is a fix that fixes mpegts startup for rtsp and any input source that didn't mark themself as non seekable.

http://pastebin.com/BnwVi08N

@elupus
Copy link
Contributor

elupus commented Dec 13, 2013

Above patch affects udp as well and makes the fixes to mark those unseekable redundant.

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

#3837 for a more complete fix. Note, it will still be slow if we can't find all stream parameters.

@margro
Copy link
Contributor Author

margro commented Dec 15, 2013

@elupus I've tested your #3837 change (taking a Gotham nightly + updated ffmpeg DLLs) together with the MediaPortal PVR addon in RTSP mode (XBMC playback of an rtsp:// URL)

#3837 results:10s switch time before => 4s after

#3590 results: 10s switch time before => 4s after, so identical results

Now with the PVR addon in TSReader mode (PVR addon sends an MPEGTS transport stream to XBMC):

#3837 results: 8s switch time before => 4s after

#3590 results: 8s switch time before => 2s after

So #3837 is indeed also a solution to reduce the switching time.

Combining #3837 with this PR did not reduce the RTSP switching time, but MPEGTS switching time is further reduced from 4s to 2s.

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

Good to hear. I've also sent #3837 upstream to ffmpeg. Not guaranteed to get in in it's current form since they might want to make it optional, since you can't force probing of whole files for further headers. But I think it has a very good chance.

The next thing we likely should add to our ffmpeg is atleast to turn on discard for the decoding. We don't have that on in our code I think. But that only reduces cpu usage somewhat.

4s still seem long. have you tested ffprobe on that file? You can apply: http://pastebin.com/nKdiWjbZ to a stock ffmpeg and do: time ffprobe -v debug [rtspurl].

Would be intersting to see the output of that when tuning.

@davilla
Copy link
Contributor

davilla commented Dec 15, 2013

if ffmpeg is still sw decoding the frames rather than decode w/skips set, an HD frame takes a long time on sw ffmpeg to decode.

@FernetMenta
Copy link
Contributor

right, on Rpi it takes ages opening a mpegts file. #3837 won't help in this case. also find_stream_info rewinds the stream to pos zero and delivers crap to decoder.

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

I've dropped the change to set AVDISCARD_ALL on startup upstream as an RFC (it breaks many many ffmpeg regression tests, so it's not a good change for us either).

Rewind is normal. We should just make sure we can rewind the full probesize in our input stream.

what do you mean delivers crap to decoder?

@FernetMenta
Copy link
Contributor

what do you mean delivers crap to decoder?

mpegts is no container and the frames prior to first IDR may not be decoded correctly. It makes no sense delivering those packets to decoders. Other players like WMC don't either.

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

that sounds more like a bug in the h264 codec. it should not send data to hw decoder that is invalid.

@davilla
Copy link
Contributor

davilla commented Dec 15, 2013

ffmpeg ASSums that it's sw/hw decoder will handle it sync to IDR. But unless you are going through an ffmpeg decoder, that does not happen. For example, rPI and AML use ffmpeg for demux only and the actual hw decode is handles elsewhere.

@davilla
Copy link
Contributor

davilla commented Dec 15, 2013

as for dropping AVDISCARD_ALL, some formats must decode the frame, all of it. Some like h264/mpeg2 can handle AVDISCARD_ALL

@margro
Copy link
Contributor Author

margro commented Dec 15, 2013

@elupus: Here the results from ffprobe (ffmpeg master) before and after #3837
https://dl.dropboxusercontent.com/u/7249985/xbmc/debuglogs/rtsp/ffmpeg_master_pull3837.txt
Those 4s include 2s for the backend to tune the channel and start the rtsp stream

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

Well. It's for h264 that the ffmpeg regression tests fail for with that
change.
On Dec 15, 2013 5:41 PM, "davilla" notifications@github.com wrote:

as for dropping AVDISCARD_ALL, some formats must decode the frame, all of
it. Some like h264/mpeg2 can handle AVDISCARD_ALL


Reply to this email directly or view it on GitHubhttps://github.com//pull/3590#issuecomment-30612767
.

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

Those logs look good to me. Takes some time before it finds the key frame
to start from. I suspect that might be possible to skip for find stream
info. It likely doesn't setup frame size until the key frame is found. It's
probably there.

Then again, shouldn't matter since we won't display anything before that
anyway.

@whaupt
Copy link
Contributor

whaupt commented Jan 20, 2014

Is there still a chance to get this one for Gotham?
Regardless of release, how do we proceed with this?

@elupus
Copy link
Contributor

elupus commented Jan 20, 2014

I'm against it in its current form. The large udp/rtsp delay has been solved in ffmpeg.

If you want to push this, please build another demuxer. I don't want this one second guessing ffmpeg.

A new mpegts demuxer would be a very good thing anyway since ffmpegs is rather bad.

@whaupt
Copy link
Contributor

whaupt commented Jan 20, 2014

OK, thx for clarification. I just wanted to know the status of this PR because there was no discussion for about a month.

@Jalle19
Copy link
Member

Jalle19 commented Jul 15, 2014

Is this still relevant with the newer ffmpeg builds we're using these days?

@FernetMenta
Copy link
Contributor

yes it is

@FernetMenta
Copy link
Contributor

see #5487

@MartijnKaijser MartijnKaijser modified the milestones: Abandoned, obsolete or superseeded, Pending for inclusion Oct 17, 2014
@margro margro deleted the gotham_channelswitchspeed branch July 19, 2015 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet