Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix replaying of videos containing one PCR wrap. #1509

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

axmhari commented Sep 30, 2012

In the current version trick speed and jumping in TS files will not work, if they contain a PCR wrap. Instead replay stops immediately. This will affect about 1 of 10 TV recordings (the TS PCR timer will overflow every 26 hours).

In the attached code this issue is fixed for one single PCR wrap, so every typical TV recording should work now.

To get it working, some changes in the ffmpeg library were necessary. I have found some discussion concerning the PCR wrap and ffmpeg on the net, but it does not seem, that there is any related work in progress.

Code is tested on Linux with numerous TV recordings and some other file formats.

Member

FernetMenta commented Sep 30, 2012

looks like a duplicate of #1227

Contributor

axmhari commented Sep 30, 2012

looks like a duplicate of #1227

You are right, I haven't seen your PR (I have to confess I just went through the more recent PRs). I will immediately try it out.

At least it underlines the need for a fix!

Member

elupus commented Sep 30, 2012

This is looking reasonable clean. Might even be accepted upstream.

@FernetMenta FernetMenta commented on the diff Sep 30, 2012

lib/ffmpeg/libavutil/avutil.h
@@ -280,6 +280,12 @@ enum AVMediaType {
#define AV_NOPTS_VALUE INT64_C(0x8000000000000000)
/**
+ * @brief Default mask for valid timestamp values
+ */
+
+#define AV_PTS_MASK INT64_C(0x7FFFFFFFFFFFFFFF)
@FernetMenta

FernetMenta Sep 30, 2012

Member

why do you need this? ffmpeg sets already pts_wrap_bits

@axmhari

axmhari Sep 30, 2012

Contributor

ffmpeg sets already pts_wrap_bits

Thanks for pointing that out, I haven't seen it. Of course this one should be used instead. I will adapt my pull request as soon as I have time.

Member

FernetMenta commented Sep 30, 2012

This looks very similar to what I had in the first approach: http://ffmpeg.org/pipermail/ffmpeg-devel/2012-August/129660.html

The change in ff_gen_seach breaks the index (follow discussion on ffmpeg mailing list)

I don't think we can expect a solution from upstream soon. This is why I have created a fallback method if search fails.

Member

elupus commented Sep 30, 2012

Yeah it's very similar i suppose. I wonder if we can solve it inside the ts
demuxer. Store first timestamp on probe, then correct output of timestamp.
I think ts wrapping once should always fit the 64bit int.

Contributor

axmhari commented Oct 1, 2012

I think I will have the new proposal ready at the weekend. It should limit the changes in ffmpeg to the ff_gen_search function. I'm still searching for a nice way to get around the ffmpeg API change in this new solution, but I already have something in mind.

I fear, that I don't get the point of the "index breaking" discussion. I tried not to treat any special cases in my code. I am just forming a linear 33 timestamp and thereby removing a possible wrap while seeking. When seeking is complete I reapply the wrap and get the correct timestamp as it is saved in the file. So for the TS case I can treat any file up to approximately 26 hours in length. If there is no PCR wrap in the file, the code should behave exactly the same way as the original version.

Anyways, please correct me if I am missing something.

Member

elupus commented Oct 1, 2012

The problem with index is that ffmpeg keeps an index of keyframes
internally. This maps a timestamps to a position in a file. This list is
expected to contain linearly increasing timestamps. It won't when the video
wraps back.

Could you see if you could fixup the timestamps in the mpegts demuxer
instead. Ie never let the wrap out of the demuxer. That may not be okey for
upstream, but it should be perfectly fine for our use case.

Contributor

axmhari commented Oct 3, 2012

Thanks, now I understand. Solving it in the mpegts demuxer sounds to be a very good idea. I will have a look at the weekend and try to get it working!

Member

elupus commented Oct 20, 2012

did you get anywhere in the mpegts.c

Contributor

axmhari commented Oct 20, 2012

Unfortunately I did not have much time for it in the last two weeks, but I got ahead today and it looks quite promising. So far it seems to be working for my TV recordings (with and without PCR wrap). However there is still some work to do, because there are code passages, which haven't been covered at all by my tests yet.

Contributor

axmhari commented Oct 21, 2012

I've now pushed the changes to my branch mpegts-pcr-wrap-fix. I only implemented the fix for the mpegts type, not for the mpegtsraw type, for which I have not found any samples to test it. I didn't find a test case for read_sl_header (for MPEG-4 generic streams?) either, but it should be okay. I will do some further testing this evening.

What do you think of it? Should I prepare a new pull request with these changes?

Member

elupus commented Oct 21, 2012

I really like the tidyness of it. It should be fine for our uses, but i
wonder what upstream will say for stream copy situations (we could likely
merge it in our "fork" anyway). Could you rebase it on ffmpeg master and
post it as an RFC on their mailing list?

Contributor

axmhari commented Oct 21, 2012

The updated pull request is available, and the patch is posted on the FFmpeg mailing list.

I assume, we can close this one.

@axmhari axmhari closed this Oct 21, 2012

@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Apr 30, 2015

@LongChair LongChair Tentative fix for crash when fixing users #1509
We might need to do this async as the window needs to be loadded first before messing with views and selected items.
d0f5592

@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Apr 30, 2015

@LongChair LongChair Dont mess with Users GUI from fetch job, should fix #1509
That seemed to happen in linux, when no it is not able to fetech users.

see https://forums.plex.tv/index.php/topic/146855-crash-in-plex-home-when-switching-users-on-ubuntu-1404/#entry851090
baa7dbb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment