[rtmpdump] Patch: improve recovery by scanning forward following packet headers to find last valid packet offset

[Martin Panter]

Hi Peter, just a couple comments below.

On 28 November 2014 at 10:19, Peter Pickford <rtmpdump at netremedies.ca> wrote:
> Hi Martin.
>
> Thanks for your feedback.
>
> rtmpdump is not the easiest of code for me to understand, its been
> quite a while since I did any c for work.
>
> The patch isn't the best c in the world, or even logic, but  it did
> fix the resumes though and if its of any use please send an an
> improved patch.
>
> My approach was to insert some code in the middle to do what I needed
> with minimum change to the rest.
>
> Apologies if my perverse way of thinking about the offsets is a bit
> perturbing but it was the best I could of think of to explain it.
>
> The idea is to find the last 'valid' tag, the last tag header where
> the pretag field is accessible. and setup tsize so the the existing
> code would scan backwards from there to the correct seekable tag and
> resume the download from there.
>
> Once it becomes necessary to scan forward from the beginning, it would
> be better to just scan forward recording the last seekable tag rather
> that the forwards and back nonsense but that was more invasive and
> given my lack of understanding of rtmpdump I though the least change
> the better.
>
> It all seems reminiscent I what we used to call relative programming
> putting in the odd +- 1 till it works.
>
> I wonder how difficult it would be to port rtmpdump to go there are
> already some go rtmp libraries, If I were to invest much more time
> into rtmpdump that is probably the way I would go.
>
> The problem(s) are that you cant look backwards if the last tag isn't
> correct and the is nothing after the last tag.
>
> I have sent a revised version that checks the current tag's previous
> tag size against the previous tags payloadsize and stops moving
> forward in the file if they don't match. It also skips that last
> couple of tags in the file, perhaps that is overkill. Unfortunately I
> only just saw your post, to many dummy emails and no forwarding etc,
> so I didn't incorporate your recommendations in the previous post I
> have removed the necessary seek in the attached patch.
>
> I wonder if some more validation would help like checking the flags
> are correct and the payload size is sensible (not sure what the
> largest sensible size would be but I suspect its a lot less than 2^24
> (or 32bits for the payloadsize).
> Once that is done the scanning backwards code could just be deleted.
>
> There is also a problem with the pause resume functionality when
> downloading from the bbc sometimes it works fine sometime you get
> garbage, perhaps if the pause resume failure can be detected switching
> to a resume strategy would speed up download progress.

I am not really familiar with the RTMP protocol or this “pause resume”
functionality, but I wonder if that problem could be fixed when the
file is being written, rather than when being resumed.

>> On 5 November 2014 00:04, Peter Pickford <rtmpdump at netremedies.ca> wrote:
>> > diff --git a/rtmpdump.c b/rtmpdump.c
>> > index 13741a7..5a69395 100644
>> > --- a/rtmpdump.c
>> > +++ b/rtmpdump.c
>> > @@ -303,8 +303,29 @@ GetLastKeyframe(FILE * file,       // output file [in]
>> >    // find the last seekable frame
>> >    off_t tsize = 0;
>> This initial value no longer used.
>> >    uint32_t prevTagSize = 0;
>> > +  uint32_t headerSize = 0;
>> > +  uint32_t payloadSize = 0;
>> > +  off_t offset = 0;
>> > +  off_t lastGoodTag = 0;
>> >
>> >    // go through the file and find the last video keyframe
>> > +  fseek(file, 5, SEEK_SET);
>> This seek shouldn’t be necessary because the flags byte at offset 4
>> was just read a few lines up.
>>
>
> True fseek to 4 and a byte is read making position 5.
>
> Removed
>
>>
>> > +  if (fread(buffer, 1, 4, file) != 4) {
>> > +       RTMP_Log(RTMP_LOGERROR, "Couldn't read headerSize from file!");
>> > +       return RD_FAILED;
>> > +  }
>> > +  headerSize = AMF_DecodeInt32(buffer);
>> > +  offset = headerSize;
>> > +  while (offset < size) {
>> What if headerSize >= size? The lastGoodTag = 0 initial value isn’t
>> going to help.
>
> If that's the case nothing is going to help the file is likely totally empty.
>
> Wouldn't this be trapped as
> "Unexpected start of file, error in tag sizes, couldn't arrive at prevTagSize=0"

Maybe so, but it seems a bit tricky to me. In other cases,
“lastGoodTag” points to a back pointer, but in this case it is zero.
What is really happening is the FLV header (or first or now second
tag) is incomplete or truncated, rather than there being an erroneous
tag size. I think a comment explaining the magic lastGoodTag = 0 value
would have helped.

>> > +       fseeko(file, offset + 5, SEEK_SET);
>> > +       if (fread(buffer, 1, 4, file) != 4) {
>> The fourth byte isn’t used is it? This alarmed me until I realized you
>> were still only decoding 3 bytes further down.
>
> Yes not that clear.
> New version reads more of the tag header.
>
>> > +               break;
>> > +       }
>> > +       payloadSize = AMF_DecodeInt24(buffer);
>> > +       lastGoodTag = offset;
>> What if the file is truncated in the middle of this “last good tag”,
>> and some of the payload is missing? Maybe you should also check that
>> the end offset (below) is within the total file size. (Unless you
>> meant this to point to the first bad tag.)
>
> Wouldn't the reading backward code look at the prevtag and seek backwards.
>
> It more of the last reachable prevtag pointer so that the original
> code can seek backwards rather than the last good tag.
> last discovered prevtag?
>
> I though something similar and tried to record the last but one tag
> header that was found.
> The read problem turned out to be that the last few tag headers have
> corrupted prevtag values!
> So I probably should strip that out out but then I have to test again
> working trumps elegant for me here as testing isn't that easy for me
> the downloads don't always fail.

I see your newer patches effectively do two new things:

1. Verify that the tag and back pointer are completely readable from
the file before blessing them as the last good tag. I think this is
good, though just checking the whole tag fits in the total file size
(offset + payloadSize + 15 + 4 <= size) would probably have been good
enough for me.

2. A sanity check comparing the previous tag size (back pointer) and
payload size. It sounds like your FLV file is getting corrupted when
the download fails, and you are relying on the sanity check to figure
out where the corruption starts.

> Its all a bit too much +4 +15 etc for my liking prevtag and payload
> not the same values, not the easiest to understand for me.
>
>> > +       offset += payloadSize + 15; // the back pointer the tag header
>> > and the payload
>> > +  }
>> > +  tsize = size - lastGoodTag - 4; // set tsize to point to offset
>> > from end of file of the header of last good tag
>> Previously “tsize” was the offset of the _end_ of the last tag from
>> EOF (i.e. zero). Now I think you are making it the offset of the
>> _start_ of the “last good tag” (i.e. end of 2nd last good tag).
>> >    do
>> >      {
>> >        int xread;