Verification required (dodgy programmer @ work)

[StainlessS]

Hi guys, just need some assurance that I'm not doing anything terribly sinful in the plug I'm putting together, the possibly
problematic bit concerns returning a string to Avisynth and handing over management of it.
The code of concern is hi-lited, the rest is still a work in progress and need not attract comment.
Also, Am I correct in thinking that the env->SaveString func makes a copy of the string and returns the new memory buffer
to the caller which can then be handed back to Avisynth and the
original buffer freed.
I would appreciate some feedback on this as stoney silence is not very reassuring.

Here's the code as it currently stands:

AVSValue __cdecl Create_RT_ReadTxtFromFile(AVSValue args, void* user_data, IScriptEnvironment* env) {
    enum{BFSZ=512};
    char bf[BFSZ+2];                                                        // with a little spare
    const char *filename=args[0].AsString();
    int Lines=args[1].AsInt(0);                                             // Allow user set default=0
    FILE *fp;   
    unsigned char c,*p,*s,*pbf;
    long flen,chk,wr,line;
    if(!(fp = fopen(filename, "r"))) {env->ThrowError("RT_ReadTxtFromFile: Cannot Open file %s",filename);} 
    if((fseek(fp, 0, SEEK_END)!=0) || ((flen=ftell(fp)) == -1L)) { fclose(fp);
        env->ThrowError("RT_ReadTxtFromFile: Cannot seek in file %s",filename);}
    rewind(fp);
    if ((chk=3)>flen)   chk=flen;
    if(chk==0) { fclose(fp); bf[0]=0; return env->SaveString(bf);}          // Return "" to Avisynth
    wr = fread(bf, 1, chk, fp);
    if(wr != chk) {fclose(fp);env->ThrowError("RT_ReadTxtFromFile: Cannot read from file %s",filename);}
    if(chk==3 && bf[0]==0xEF && bf[1]==0xBB && bf[2]==0xBF) {
        fclose(fp); env->ThrowError("RT_ReadTxtFromFile: UTF-8 text files are not supported, "
        "re-save with ANSI encoding! : '%s'", filename);}
    if((chk>=2 && ((bf[0]==0xFF && bf[1]==0xFE) || (bf[0]==0xFE && bf[1]==0xFF)))) {
        fclose(fp); env->ThrowError("RT_ReadTxtFromFile: UTF-8 text files are not supported, "
        "re-save with ANSI encoding! : '%s'", filename);}
    rewind(fp);
    if((pbf = new unsigned char[flen])==NULL)
        {fclose(fp); env->ThrowError("RT_ReadTxtFromFile: Cannot allocate memory for %s",filename);}
    p=pbf;
    if(Lines <= 0) Lines = 0x7FFFFFFF;
    line = 0;
    while(line < Lines && (fgets(bf, BFSZ,fp ) != NULL)) {                  // will halt at first Nul byte if bin file
        ++line;
        s = (unsigned char *)bf;                                            // start of input buffer
        while(c = *s) {
            if(c>=' ' && c <= 126) {                                        // All ASCII printable incl SPACE
                *p++=c;
            } else if(c=='\n' || c=='\r') {
                *p++='\n';                                                  // Output single '\n'
                if((c=='\n' && s[1]=='\r') || (c=='\r' &&  s[1]=='\n'))     // Allow skip of matched pairs CR/LF or LF/CR
                    ++s;                                                    // Skip to 1st of match pair EOL
            } else { // Just allow control codes and non Ascii through, maybe change in future, perhaps user selectable
                *p++ = c;                                                   
            }
            ++s;                                                            // Next char in input buffer
        }
    }           
    *p='\0';                                                                // Nul term sentinel
    fclose(fp);
    AVSValue ret =  env->SaveString((const char*)pbf);                      // Get pointer to Avisynth saved string
    delete [] pbf;                                                          // Delete original buffer
    return ret;                                                             // Return Avisynth's copy to Avisynth
}

EDITED:

and the calling script

TS=RT_ReadTxtFromFile("D:\avs\bank\mcdegrain.avs")
RT_debug(TS)
TS="" # Hopefully this will deallocate the string.
EDIT: This would have been a better choice: TS=0

And the output in RT_Stats RT_Debug()

00000004  02:32:34  RT_Debug: 
00000005  02:32:34  RT_Debug: Function MCDegrain(clip c, int "frames")  
00000006  02:32:34  RT_Debug: { # By Didee, http://forum.doom9.org/showthread.php?p=1508289#post1508289 
00000007  02:32:34  RT_Debug: 
00000008  02:32:34  RT_Debug: frames = default(frames, 2) 
00000009  02:32:34  RT_Debug: bs = (c.width>960) ? 16 : 8 
00000010  02:32:34  RT_Debug: super = c.MSuper(pel=2, sharp=1)  
00000011  02:32:34  RT_Debug: backward_vec3 = MAnalyse(super, isb = true, delta = 3, blksize=bs, overlap=bs/2)  
00000012  02:32:34  RT_Debug: backward_vec2 = MAnalyse(super, isb = true, delta = 2, blksize=bs, overlap=bs/2)  
00000013  02:32:34  RT_Debug: backward_vec1 = MAnalyse(super, isb = true, delta = 1, blksize=bs, overlap=bs/2)  
00000014  02:32:34  RT_Debug: forward_vec1 = MAnalyse(super, isb = false, delta = 1, blksize=bs, overlap=bs/2)  
00000015  02:32:34  RT_Debug: forward_vec2 = MAnalyse(super, isb = false, delta = 2, blksize=bs, overlap=bs/2)  
00000016  02:32:34  RT_Debug: forward_vec3 = MAnalyse(super, isb = false, delta = 3, blksize=bs, overlap=bs/2)  
00000017  02:32:34  RT_Debug: (frames<=0) ? c :\  
00000018  02:32:34  RT_Debug: (frames==1) ? c.MDegrain1(super, backward_vec1,forward_vec1,thSAD=400) :\ 
00000019  02:32:34  RT_Debug: (frames==2) ? c.MDegrain2(super, backward_vec1,forward_vec1,backward_vec2,forward_vec2,thSAD=400) :\  
00000020  02:32:34  RT_Debug:      c.MDegrain3(super, backward_vec1,forward_vec1,backward_vec2,forward_vec2,backward_vec3,forward_vec3,thSAD=400) 
00000021  02:32:34  RT_Debug: return(last)  
00000022  02:32:34  RT_Debug: } 
00000023  02:32:34  RT_Debug: 
00000024  02:32:34  RT_Debug: Function MCDegrainSharp(clip c, int "frames", float "bblur", float "csharp", bool "bsrch")  
00000025  02:32:34  RT_Debug: { # Based on MCDegrain By Didee, http://forum.doom9.org/showthread.php?t=161594 
00000026  02:32:34  RT_Debug:  # Also based on DiDee observations in this thread: http://forum.doom9.org/showthread.php?t=161580  
00000027  02:32:34  RT_Debug:  # "Denoise with MDegrainX, do slight sharpening where motionmatch is good, do slight blurring where motionmatch is bad"  
00000028  02:32:34  RT_Debug:  # In areas where MAnalyse cannot find good matches, the blur() will be dominant. 
00000029  02:32:34  RT_Debug:  # In areas where good matches are found, the sharpen()'ed pixels will overweight the blur()'ed pixels  
00000030  02:32:34  RT_Debug:  # when the pixel averaging is performed.   
00000031  02:32:34  RT_Debug: frames = default(frames, 2) 
00000032  02:32:34  RT_Debug: bblur  = default(bblur, 0.6)  
00000033  02:32:34  RT_Debug: csharp = default(csharp, 0.6) 
00000034  02:32:34  RT_Debug: bsrch  = default(bsrch, true) 
00000035  02:32:34  RT_Debug: bs = (c.width>960) ? 16 : 8 
00000036  02:32:34  RT_Debug: 
00000037  02:32:34  RT_Debug: c2 = c.blur(bblur)  
00000038  02:32:34  RT_Debug: super = bsrch ? c2.MSuper(pel=2, sharp=1) : c.MSuper(pel=2, sharp=1)  
00000039  02:32:34  RT_Debug: super_rend = c.sharpen(csharp).MSuper(pel=2, sharp=1,levels=1)  
00000040  02:32:34  RT_Debug: backward_vec3 = MAnalyse(super, isb = true, delta = 3, blksize=bs, overlap=bs/2)  
00000041  02:32:34  RT_Debug: backward_vec2 = MAnalyse(super, isb = true, delta = 2, blksize=bs, overlap=bs/2)  
00000042  02:32:34  RT_Debug: backward_vec1 = MAnalyse(super, isb = true, delta = 1, blksize=bs, overlap=bs/2)  
00000043  02:32:34  RT_Debug: forward_vec1 = MAnalyse(super, isb = false, delta = 1, blksize=bs, overlap=bs/2)  
00000044  02:32:34  RT_Debug: forward_vec2 = MAnalyse(super, isb = false, delta = 2, blksize=bs, overlap=bs/2)  
00000045  02:32:34  RT_Debug: forward_vec3 = MAnalyse(super, isb = false, delta = 3, blksize=bs, overlap=bs/2)  
00000046  02:32:34  RT_Debug: (frames<=0) ? c :\  
00000047  02:32:34  RT_Debug: (frames==1) ? c2.MDegrain1(super_rend, backward_vec1,forward_vec1,thSAD=400) :\ 
00000048  02:32:34  RT_Debug: (frames==2) ? c2.MDegrain2(super_rend, backward_vec1,forward_vec1,backward_vec2,forward_vec2,thSAD=400) :\  
00000049  02:32:34  RT_Debug:      c2.MDegrain3(super_rend, backward_vec1,forward_vec1,backward_vec2,forward_vec2,backward_vec3,forward_vec3,thSAD=400) 
00000050  02:32:34  RT_Debug: return(last)  
00000051  02:32:34  RT_Debug: } 
00000052  02:32:34  RT_Debug: 
00000053  02:32:34  RT_Debug:

The New plug is to read in a text file and return it in a string,
and work being done on RT_Debug, will parse a string for EOL codes and output to OutputDebugString.

EDIT:
RT_Stats v1.02, below of from mediafire account in sig:
http://forum.doom9.org/showthread.php?t=165479
EDIT: Above link fixed.

[IanB]

env->SaveString(data) copies the string data into the strings save space of the current IScriptEnvironment. The string save space is released only when the current IScriptEnvironment is deleted.

So no,
TS="" # Hopefully this will deallocate the string.
will not release the memory formally pointed to by TS, it just sets TS to point to another string, i.e. "".

For text string data, env->SaveString(data), is a simple and convenient way to do semi-static allocation.

For more complex resources you can register a clean up handler with env->AtExit(). Again this is run when the current IScriptEnvironment is deleted.


Not sure what you are trying to achieve here, but it get a hint you are still struggling with the distinction between compile time and execution time. The goal of compiling a script is to produce a final complex object with a GetFrame() method and a GetAudio() method that the host application can call to retrieve video frames and audio data respectively. Having arbitrary things happen while the script is compiling can be a useful thing, but it should not distract from the final goal. If your final goal is not to have something make GetFrame() and/or GetAudio() calls, then Avisynth is possibly not the right tool for the task.

[StainlessS]

Thank you for you answer, must assume that all internally managed strings are likewise accumulated until release of current script environment.

The purpose of the RT stuff is ultimately to provide a more automatic solution to video format conversion, so that operations usually
done by a person can be automated prior to frame serving. Being able to eg extract the first few lines from a d2v file to ascertain
eg full frame Display Aspect Ratio or whatever, can be useful and reduces the amount of that personal touch needed.

I dont think its such a bad thing to have options.

Again, thanks for the response.

EDIT: I guess that NOT using env->SaveString and just forgetting altogether about delete-ing the space is pretty
much OK too and will be removed when ALL Environments disappear.
Am now happier I think I fully understand, although a little memory garbage collection might be a worth considering.
EDIT:
env->SaveString actually means, "delete string When current environment closes down". ?

[IanB]

Originally Posted by StainlessS

must assume that all internally managed strings are likewise accumulated until release of current script environment.

Yes.

EDIT: I guess that NOT using env->SaveString and just forgetting altogether about delete-ing the space is pretty
much OK too and will be removed when ALL Environments disappear.

Not deleting memory is called leaking and is most likely a bad thing. Avisynth is a library and at the beck and call of the parent application. Think along the lines of VirtualDub or other program running a big batch of queued jobs. It does not exit between jobs. Each IScriptEnvironment is created at the start and deleted at the end of each job.

Am now happier I think I fully understand, although a little memory garbage collection might be a worth considering.

Yeah, maybe reference counting strings could be done, but would be a lot of work to change the code. Even for excessive scripts the string space is usually 100's of Kilobytes, compared to Megabytes for video frames.

EDIT: env->SaveString actually means, "delete string When current environment closes down". ?

Yes, or "Save the string until the IScriptEnvironment closes down".

[StainlessS]

Thanks.