Modify
Warning Please read the Ticket HowTo before creating or commenting on a ticket. Failure to do so may cause your ticket to be rejected or result in a slower response.

Opened 3 years ago

Closed 3 years ago

Last modified 3 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 3 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@… 3 years ago.
Patch refered to from Comment 5. Includes previous patch
fix9410-bbc-hd2.patch (16.5 KB) - added by mythtv@… 3 years ago.
New patch containing all changes
fix9410-bbc-hd3.patch (18.6 KB) - added by mythtv@… 3 years ago.
New version with some tidying. Also now reads multiple SEI messages
fix9410-bbc-hd3b.patch (19.4 KB) - added by jpoet 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by mythtv@…

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

comment:2 Changed 3 years ago by tralph

  • Status changed from new to assigned

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 3 years ago by jpoet

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

comment:3 Changed 3 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 3 years ago by tralph

  • Milestone changed from unknown to 0.24.1
  • Priority changed from minor to critical

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

comment:5 Changed 3 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 3 years ago by mythtv@…

Patch refered to from Comment 5. Includes previous patch

comment:6 Changed 3 years ago by jpoet

  • Owner changed from janne to jpoet
  • Status changed from assigned to accepted

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 3 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 3 years ago by mythtv@…

New patch containing all changes

comment:8 Changed 3 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 3 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 3 years ago by mythtv@…

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

Changed 3 years ago by jpoet

comment:10 Changed 3 years ago by beirdo

See also #9176

comment:11 Changed 3 years ago by jpoet

  • Resolution set to Fixed
  • Status changed from accepted to closed

Fixed by c9c2545ac

comment:12 Changed 3 years ago by John Poet

  • Resolution changed from Fixed to fixed

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

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'new'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.