Adding a track with no name prevents opening file with mp4v2 version 1.6 (old sourceforge version)

[GoogleCodeExporter]

What steps will reproduce the problem?
1. fh = MP4Modify(fn); //fn is an iTunes music file, or other file with no 
track name.
2. dstTrackId = MP4CopyTrack(fh, 1);
3. MP4DeleteTrack(fh, 1);
4. MP4Close(fh);
5. Run version 1.6 mp4dump command on the modified file.

What is the expected output? What do you see instead?
Expected output: mp4dump should print a dump of the file.
Observed output: mp4dump dies with "atom too small" error in file mp4atom.cpp 
line 586 (line number refers to version 1.6 of file).

What version of the product are you using? On what operating system?
Version mp4v2-trunk-r355 on Windows XP.

Please provide any additional information below.
The problem is caused by MP4File::AddAudioTrack line 1264, which adds an 
unnamed udta.name atom. If the name is not later set, this leaves a zero-length 
value in the file, which crashes the old version 1.6 (on sourceforge) mp4v2 
during initial parsing of the file. This means that programs built with the old 
version of mp4v2 (such as aacgain) can not open these files created with the 
new version.

Although the resulting file is legal mp4, I am questioning the wisdom of 
putting line 1264 in the code. If the track is named, then calling 
MP4SetTrackName will create a udta.name atom. If the track is not named, then 
what is the purpose of the empty atom?

History of file mp4file.cpp shows that this code was added in version r14 by 
jb.rubin, to make it consistent with AddAC3AudioTrack which was added in 
version r9 by eddygroenendaal for handbrake support.

If there is a good reason why that line of code is there, please let me know, 
otherwise I would suggest it be removed from all AddXXXTrack methods.

Original issue reported on code.google.com by davelas...@gmail.com on 28 Jul 2010 at 7:45

[GoogleCodeExporter]

I'm not totally clear on why the code is there, but I believe Issue #9 is 
somehow related to the trak.udta.name problem you're having.

Also, I see that mp4track --udtaname-remove can be used to "remove 
trak.udta.name atom. This action will remove the optional atom."  If you don't 
want to use mp4track, you could pretty easily call something similar to:

void
TrackModifier::removeUserDataName()
{
    MP4Atom* name = _track.FindAtom( "trak.udta.name" );
    if( name )
        name->GetParentAtom()->DeleteChildAtom( name );

    MP4Atom* udta = _track.FindAtom( "trak.udta" );
    if( udta && !udta->GetNumberOfChildAtoms() )
        udta->GetParentAtom()->DeleteChildAtom( udta );
}

...to zap it.  I might need Kona or Eddy to comment on this one; I'm really not 
clear on why that code was added, what ends it serves, or what possible 
ramifications there are to removing it.

Original comment by kid...@gmail.com on 7 Oct 2010 at 2:41

[GoogleCodeExporter]

My reading of Issue 9 is that it refers to the case where the name is not 
empty. This issue refers to the case of an empty name.

I agree I could code something similar to TrackModifier::removeUserDataName, 
however, I am trying to stick to using the C API. I don't think the workaround 
is possible without using C++.

Original comment by davelas...@gmail.com on 7 Oct 2010 at 3:06

[GoogleCodeExporter]

I think you're right about issue 9; still, it's not clear to me why they added 
that atom.  I've seen other indications floating about that trak.udta.name is 
superfluous, like http://forum.doom9.org/showthread.php?p=1405402#post1405402 . 
 I can't find mention of it anywhere in the standard.

What do you think the risks are to removing this?

Original comment by kid...@gmail.com on 7 Oct 2010 at 9:05

[GoogleCodeExporter]

I don't know the standard or the mp4v2 code well enough to assess the risk of 
removing the unnamed atom. Is there any way you can track down Eddy? He appears 
to be the original author of that line of code.

Original comment by davelas...@gmail.com on 7 Oct 2010 at 9:44

[GoogleCodeExporter]

This sounds like broken code from mp4v2 1.6 and earlier. But mp4v2 can be 
improved to be more clean in this particular case which avoids the 1.6 bug 
altogether.

udta (user data atom) is actually defined in ISO 14496-12 but they don't go 
into the messy business of defining known child atoms and instead only show 
cprt (copyright) as but one example.

udta can be contained inside moov and trak in both ISO (mp4) and QTFF 
(quicktime mov) files. QTFF specifies a large number of child atoms and their 
descriptions, one of which is name. And it is very generic but the common usage 
that I've seen is to have trak.udta.name atoms used as an informative name of 
the track. So for example, "stereo" or "commentary" might be used. But it could 
just as easily be "main - stereo" and "commentary - mono". It's really up to 
the muxing application.

The important thing to remember with udta is that it is optional, as are it's 
children and give that name is so very free-form and typically meant to be 
interpreted by a human and not programatically, I'd recommend:

in MP4File::FinishWrite()
  for moov.udta.name and moov.trak[*].udta.name
    if udta.name.value is empty, remove udta.name
    if udta has no children, remove udta

This will ensure blank udta.name doesn't exist - which is pretty useless 
anyways, and also extra cleanup by removing udta if it's empty.

Just to cover some basic testing, try viewing the resulting file with 
Quicktime7 Pro properties inspector. Its behaviour is to display trak.udta.name 
or some default if not present. I don't think QuicktimeX or VLC do/display 
anything with udta.name but try there too.

Original comment by Kona8l...@gmail.com on 8 Oct 2010 at 3:55

[GoogleCodeExporter]

Kona,

Thanks for the info, that was a big help.  It does sound like broken code in 
mp4v2 1.6, but it also sounds like an opportunity to clean up useless atoms and 
gain a bit of backwards compatibility at the same time.  So I went ahead and 
added the code in r395 - a review would be nice if you get a chance.  However, 
I don't have Quicktime7 Pro or QuicktimeX, so I'll have to verify that later.  
I did test my code to ensure it did not remove udta.name atoms with non zero 
values (and of course, removed the atom if it was zero-length).  I'll see if I 
can pick Qt7 up tomorrow, or if you get a chance, it'd be great if you could 
test that.

Dave, could you please retest r395 on your end?  

Original comment by kid...@gmail.com on 8 Oct 2010 at 7:10

• Changed state: Fixed

[GoogleCodeExporter]

Made one more change--please test r396 if you can.  Thanks!

Original comment by kid...@gmail.com on 8 Oct 2010 at 6:35

[GoogleCodeExporter]

I can confirm that r396 fixes the problem. Thanks for the help!

Original comment by davelas...@gmail.com on 10 Oct 2010 at 1:16