Doc for MP4Optimze incorrect?

[GoogleCodeExporter]

The doc for MP4Optimize says:

Parameters:
        fileName    pathname of (existing) file to be optimized.
        newFileName     pathname of the new optimized file. If NULL a temporary file will be used and fileName will be over-written upon successful completion.
        verbosity   bitmask of diagnostic details the library should print to stdout during its functioning.

but at the beginning of the function it has:
if (!existingFileName || !newFileName)
            return false;

So if I want to optimize the file in place, should I just use the same filename 
for both fileName and newFileName?

Original issue reported on code.google.com by danahins...@gmail.com on 27 May 2011 at 1:19

[GoogleCodeExporter]

I just tried it supplying the same name for fileName and newFileName and it 
doesn't optimize the file (even though it doesn't return failure either).  So I 
guess I have to create the temp filename and delete and rename it on my own?

Original comment by danahins...@gmail.com on 27 May 2011 at 1:36

[GoogleCodeExporter]

That looks like a regression in r453...let me look closer.  Thanks for the 
report.

Original comment by kid...@gmail.com on 27 May 2011 at 8:16

[GoogleCodeExporter]

Doc is correct, and I actually did a bunch of work to improve the in-place 
optimize.  An incorrect check was added over in the logging branch and then got 
merged--should be fixed in r467.  Let me know if you still have problems.

Original comment by kid...@gmail.com on 27 May 2011 at 8:32

• Changed state: Fixed

[GoogleCodeExporter]

Sorry for screwing that up.  Thanks for catching it.

Original comment by dbyr...@gmail.com on 27 May 2011 at 8:36

[GoogleCodeExporter]

No worries, easy to miss ;)

Original comment by kid...@gmail.com on 27 May 2011 at 8:37

[GoogleCodeExporter]

Well, it still doesn't work for me (after resyncing and rebuilding.  The 
erroneous check is now gone, but when I get to MP4File::Optimize and it tries 
to open the dest files with:

        Open( dname.c_str(), File::MODE_CREATE, NULL );

it fails with errcode 32, and then it has a catch with a throw, and in 
MP4Optimize it doesn't even return a bad return so you can't tell from the call 
that it failed.

And another thing I noticed is that I get a debug output:
mp4v2::impl::MP4TrackArray::operator []: illegal array index: 2 of 2: errno: 34 
(c:\source\mp4v2\src\mp4track.h,288)

Not sure it's it's related or something different.

FWIW I'm running on Windows 7 64 bit.

Dan

Original comment by danahins...@gmail.com on 27 May 2011 at 10:22

[GoogleCodeExporter]

From my reading of src/mp4file.cpp, if the call to pFile->Optimize() throws 
(line 236 for me), then MP4Optimize returns false (the last line of the 
function).  Is that not what you see?

If not, then perhaps the MP4TrackArray illegal array index is relevant.

Original comment by dbyr...@gmail.com on 30 May 2011 at 12:02

[GoogleCodeExporter]

OK, there's something strange between C++ and VB.  I declare the function to 
return a boolean in VB, I trace into the C++ code and see it return false, but 
when I get back to VB it's returned true (I tried it returning true, just to 
make sure it wasn't reversing it for some reason).  So I'll continue to pursue 
this and see if I can figure out what's going on with the return code.

After re-reading the doc (and the code) I see that if I want to optimize it in 
place, I need to pass in null for the destination, not have src = dest.  It 
would be nice for the api to just recognize this and optimize it in place, but 
I'll change my code.

Still not sure what the mp4trackarray issue is.  I took a quick look at the 
macro, but couldn't tell what the complaint meant.

Original comment by danahins...@gmail.com on 30 May 2011 at 4:58

[GoogleCodeExporter]

I spoke too soon, after it optimizes the file, it tries to move the temp file 
back to the original with:

    // move temporary file into place
    if( !dstFileName )
        Rename( dname.c_str(), srcFileName );

but since the original file is still there it fails.  So you're left with the 
original file and the optimized file with the temporary name.

Original comment by danahins...@gmail.com on 30 May 2011 at 5:07

[GoogleCodeExporter]

The call to Rename is really MP4File::Rename which calls FileSystem::rename.  
libplatform/io/FileSystem.h says that FileSystem::rename must remove a file 
with the destination name if it exists.

The Windows implementation of FileSystem::rename (in 
libplatform/io/FileSystem_win32.cpp) calls MoveFileExW with the 
MOVEFILE_REPLACE_EXISTING flag which, according to 
http://msdn.microsoft.com/en-us/library/aa365240%28v=vs.85%29.aspx, should do 
the trick.

If the rename operation fails, I expect to see a log message.  Do you?

Original comment by dbyr...@gmail.com on 30 May 2011 at 5:32

[GoogleCodeExporter]

I think the problem is in Filesystem::rename, where it starts with:

    if (!from_file.IsUTF16Valid() || to_file.IsUTF16Valid())
    {
        return true;
    }

I think you want the ! in front of the to_file.IsUTF16Valid()

Original comment by danahins...@gmail.com on 30 May 2011 at 6:52

[GoogleCodeExporter]

I agree.  Changed in svn r469.

Original comment by dbyr...@gmail.com on 30 May 2011 at 7:07