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.
Some responses inline.
Thanks
Peter
> I was just looking at this code (considering a resume function for a
> HDS downloader), so I thought I would try and understand your patch as
> well. It looks like this patch is intended to help resume a file with
> an incomplete FLV tag at the end. Some comments below; hope they are
> useful.
> On 5 November 2014 00:04, Peter Pickford <rtmpdump(a)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"
> > + 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.
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;
