Review Board 2.0.15


VNC: Add support for capturing frame buffer to file each time it is changed.

Review Request #631 - Created April 4, 2011 and discarded

Information
Ali Saidi
gem5
Reviewers
Default
ali, gblack, nate, stever
VNC: Add support for capturing frame buffer to file each time it is changed.

When a change in the frame buffer from the VNC server is detected, the new
frame is stored out to the m5out/frames_*/ directory.  Specifiy the flag
"--frame-capture" when running configs/example/fs.py to enable this behavior.

   
Posted (April 12, 2011, 6:38 p.m.)
At a high level there are really two changes here, some renovation of the output directory class, and then the actual VNC stuff, and they may be of interest to different people. I'd be a lot more comfortable with the output directory part if Nate was ok with it, for instance, but he might not even know those files are involved from the change description.
src/base/bitmap.hh (Diff revision 2)
 
 
This will work, I think, but why not just keep a flag around that you set when somebody changes some bytes in the buffer? I don't think there's a way for them to do that that you wouldn't see. hashing the whole image over and over is probably pretty expensive.
src/base/bitmap.cc (Diff revision 2)
 
 
memcpy and these casts make me nervous. I think the casts are probably unnecessary since memcpy takes void*s and I'm pretty sure any pointer can be implicitly cast into a void *.
Posted (April 13, 2011, 7:28 a.m.)
Can you separate the output directory stuff for the next version of the diff.  Overall, I'm ok with this code, though I'd rather see us move it into python some day.  Python would certainly make it a lot simpler.
src/base/output.hh (Diff revision 2)
 
 
heh, pedantic.  I guess we may some day run on another OS. :)
src/base/output.hh (Diff revision 2)
 
 
You should get rid of this, use the const version you added below everywhere and require the user to call create if find returns null.  Requires changes elsewhere though.  Create on side effect was a bad idea.
src/base/output.cc (Diff revision 2)
 
 
Please use:

if (i->second != openStream)
    continue;

and remove a level of indent.

I know this isn't in the style guide, but it definitely leads to more readable code.
src/base/output.cc (Diff revision 2)
 
 
What's the point of the else block if the if (fs) has a "break;" in it?
src/base/output.cc (Diff revision 2)
 
 
If you choose not to get rid of this version of the function, it should at least be based on the const version and not duplicate code.
src/base/output.cc (Diff revision 2)
 
 
Wondering if we should give these more standard names like "mkdir"
src/base/output.cc (Diff revision 2)
 
 
Could this happen?  Would it resolve? Shouldn't we prevent this on the creation side or in resolve itself?