Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#9410 closed Bug Report (fixed)

Positioning problems with BBC HD and BBC ONE HD recordings

Reported by: mythtv@… Owned by: jpoet
Priority: critical Milestone: 0.24.1
Component: MythTV - Video Playback Version: Unspecified
Severity: medium Keywords:
Cc: Ticket locked: no

Description

BBC HD and BBC ONE HD recordings have their lengths reported incorrectly. Also use of bookmarks and uses of forward/backward jumps tend to move to positions ahead of the correct one (this happens only in playback via MythFronend?: bookmarks work correctly if a recording is copied to MythVideo?).

ffmpeg report the framerate incorrectly from such files - giving say 27.45 where it should be 25.0 - which could be connected to the problem. It has been suggested that it is a flaw in the mythtv h264 parer. There has been a discussion here http://www.gossamer-threads.com/lists/mythtv/users/462093

I've uploaded a short sample here: http://download.glidos.net/ALittleLaterDuffyPart.mpg. I can upload that complete recording if necessary.

Attachments (5)

H264Parser-nal-ref-idc.patch (4.3 KB) - added by jpoet 13 years ago.
Only test nal_ref_idc for a new AU, if the NAL UNIT is a "slice"
fix9410-bbc-hd.patch (8.4 KB) - added by mythtv@… 13 years ago.
Patch refered to from Comment 5. Includes previous patch
fix9410-bbc-hd2.patch (16.5 KB) - added by mythtv@… 13 years ago.
New patch containing all changes
fix9410-bbc-hd3.patch (18.6 KB) - added by mythtv@… 13 years ago.
New version with some tidying. Also now reads multiple SEI messages
fix9410-bbc-hd3b.patch (19.4 KB) - added by jpoet 13 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 13 years ago by mythtv@…

Here is another sample provided by David Knight: http://caprica.unospace.net/8941_20101127194500-sample.mpg

comment:2 Changed 13 years ago by tralph

Status: newassigned

I've confirmed this is related to the h264 parser not detecting on_frame properly while playing back or running commercial detection.

A temporary work around until this is fixed would be to return true always for H264PreProcessPkt in avforatdecoder.cpp and rerunning commercial flagging.

Changed 13 years ago by jpoet

Only test nal_ref_idc for a new AU, if the NAL UNIT is a "slice"

comment:3 Changed 13 years ago by jpoet

When trying to detect a "new" access unit (AU), one of the things to look for is a change in the nal_ref_idc value.

nal_ref_idc is part of every NAL UNIT, and we are currently testing to see if it changed for every NAL UNIT. Re-reading T-REC-H.264-200711-I!!PDF-E.pdf, I don't think that is correct. When looking for a "new" AU, the nal_ref_idc should only be tested when the current NAL UNIT is a "slice".

I have attached a patch which makes this change. I have tested it with HD-PVR recordings, and it does not seem to cause anything bad to happen.

It would be nice if someone else would read the spec to see if they interpret it the same way.

comment:4 Changed 13 years ago by tralph

Milestone: unknown0.24.1
Priority: minorcritical

Increasing priority since this greatly impacts recordings from a major broadcaster.

comment:5 Changed 13 years ago by mythtv@…

I now have at least a partial fix for this, in that I've found two aspects of the spec not implemented, and added code to handle them. Results, after making these changes (plus one of John Poet's), and then running "mythcommflag --rebuild", look promising. Most of the recordings I've tested now search perfectly. I did have some strange behaviour during Live playback of the news this evening, so there may be further problems to address.

There are three changes:

1) Prior to parsing the bodies of the various NALs, remove any emulation-prevention bytes. This should improve results for all H264, not just BBC HD. To keep the initial implementation simple, I've had to allocate a buffer to copy into, while performing the removal. It might be better, at some stage, to make a version of the getbits library that can jump over these bytes as it goes, but that would be quite difficult. I also removed all increments to byteP: provided it is incremented past the start code, there is no need to move it any further, and to do so carries the risk of miscalculating and moving it too far.

2) I've updated the code for reading profile>=100 versions of the SPS. The scaling-list-present flag was being read, but not the lists themselves, and since the log2_max_frame_num_minus4 field occurs after, frame numbers were not being read correctly from slices. That was probably the biggest contributor to BBC HD problems.

3) I've added John Poet's change that looks for nal_ref_idc changes only in slices.

The three change are in my fork at: https://github.com/Glidos/mythtv They are the last three commits on branch fix9410-bbc-hd. I'm also attaching a patch.

Changed 13 years ago by mythtv@…

Attachment: fix9410-bbc-hd.patch added

Patch refered to from Comment 5. Includes previous patch

comment:6 Changed 13 years ago by jpoet

Owner: changed from Janne Grunau to jpoet
Status: assignedaccepted

I have not looked at the code yet, but I had a spare hour so I compiled it up and tried to record some content from my HD-PVR. Unfortunately, with the patch I get zero-byte files. I am guessing that it is not detecting the HD-PVR's keyframes correctly.

I should have some time on Sunday to dig further.

comment:7 Changed 13 years ago by mythtv@…

I've prepared a new version which removes all assumptions of alignment between NALs and the packets passed into the parser (for the entire change, see the four commits between fixes/0.24 andfix9410-bbc-hd in my fork at https://github.com/Glidos/mythtv, or the patch I'll upload in the next comment). I've added debug to both this version and the previous version, and they give identical results on a BBC HD sample, when run with John's test program, passing 128KB chunks. With this new version I've also tried with 128B chunks passed in, and that still gives identical results (which was the whole point of the change).

With the previous version, I was seeing the recordedmarkup table filling up with huge numbers of width/height/aspect records during recording (eventually leading to mythbackend stalling). I guessed this was because the SPS was not being passed in a single continuous chunk. The guess seems to be correct, because now I see a single record added with sensible values. In any case, the dtvrecorder class looks to pass in a TS packet at a time, rather than building up PES packets; avformatdecoder, on the other hand, looks to build up packets, which would explain why "mythcommflag --rebuild" would work and recording not. Also, H.222 specifies that there are no assumptions of alignment between NALs and PES packets unless a certain flag is set.

I haven't had time yet to check a sample of John Poet's problematic HD-PVR recording, but I'm hoping that this new version might be a fix. I'm possibly now seeing problems with ITV HD recordings, but I need to do more testing. I have a past recording that now wont play. I used the test program to pass it through the parser, and it found frames about every 50KB, with every 32nd being a key frame, so it looks to be working correctly. I hope it's not a nasty side-effect of the fact that now we now often leave parsing a NAL until finding the start code of the next.

Changed 13 years ago by mythtv@…

Attachment: fix9410-bbc-hd2.patch added

New patch containing all changes

comment:8 Changed 13 years ago by mythtv@…

Further testing shows no problems with ITV HD after all. I've regenerated the seek table for the old recorded show that would no longer play back, and it's fine again. I've also recorded several ITV HD shows and they seem fine too. I've recorded a number of BBC HD shows too, without any of the problems of multiple recordedmarkup records, and they look to seek perfectly.

If this latest patch works with HD-PVR recordings then perhaps this is fixed now.

comment:9 Changed 13 years ago by mythtv@…

fix9410-bbc-hd2.patch has been working fine for me, but there were a few things I wanted to tidy up, so I'm uploading a new patch.

Changed 13 years ago by mythtv@…

Attachment: fix9410-bbc-hd3.patch added

New version with some tidying. Also now reads multiple SEI messages

Changed 13 years ago by jpoet

Attachment: fix9410-bbc-hd3b.patch added

comment:10 Changed 13 years ago by beirdo

See also #9176

comment:11 Changed 13 years ago by jpoet

Resolution: Fixed
Status: acceptedclosed

Fixed by c9c2545ac

comment:12 Changed 13 years ago by John Poet

Resolution: Fixedfixed

Fix BBC frame detection.

Patch is by Paul Gardiner with some very minor tweaks by me.

Paul's comments:

Remove all assumptions of alignment between NALs and the packets passed into the parser.

Prior to parsing the bodies of the various NALs, remove any emulation-prevention bytes. This should improve results for all H264, not just BBC HD.

Update the code for reading profile>=100 versions of the SPS. The scaling-list-present flag was being read, but not the lists themselves, and since the log2_max_frame_num_minus4 field occurs after, frame numbers were not being read correctly from slices. That was probably the biggest contributor to BBC HD problems.

Fixes #9410

Changeset: c9c2545ac67584af5e8d705486bbb294f3e7da50

Note: See TracTickets for help on using tickets.