On Thursday 13 March 2008 09:01:53 pm Maarten Lankhorst wrote:
If read from a file source, ID3 v2 tags might be incorrectly parsed by the head_check. This patch skips the whole tag altogether.
I'm not sure this is completely correct. The main splitter function detects the presense of an MP3 header at the start of a sample it's sending to set the sample's sync point attribute. Since the ID3 tag is not a sync point, this causes the first sample to not be set as a sync point, and it would be unlikely that a following sample would, under normal circumstances, contain an MP3 header at the start either. This could cause sensitive codecs to never find a sync point.
ACMWrapper wouldn't be affected as it doesn't handle sync points, but other native codecs (which do work in Wine) could. Additionally, winemp3.acm may be affected by the ID3 tag as it doesn't seem to have code to handle it either.
Hi Chris,
2008/3/14, Chris Robinson chris.kcat@gmail.com:
On Thursday 13 March 2008 09:01:53 pm Maarten Lankhorst wrote:
If read from a file source, ID3 v2 tags might be incorrectly parsed by the head_check. This patch skips the whole tag altogether.
I'm not sure this is completely correct. The main splitter function detects the presense of an MP3 header at the start of a sample it's sending to set the sample's sync point attribute. Since the ID3 tag is not a sync point, this causes the first sample to not be set as a sync point, and it would be unlikely that a following sample would, under normal circumstances, contain an MP3 header at the start either. This could cause sensitive codecs to never find a sync point.
In general the tag is not a sync point, but some data behind it accidentally may be parsed as such. Even if it didn't, the tag is generally a few 100 (or about 1000 in my case) bytes in size. Reading all those bytes individually and moving them all 4 times doesn't seem really optimal. Skipping the amount of data that is set in the size buffer causes it to immediately find the first mp3 header.
ACMWrapper wouldn't be affected as it doesn't handle sync points, but other native codecs (which do work in Wine) could. Additionally, winemp3.acm may be affected by the ID3 tag as it doesn't seem to have code to handle it either.
It doesn't affect sync points, everything in the id3 tag is garbage and skipping it is a good thing (TM).
Cheers, Maarten.
On Friday 14 March 2008 10:29:34 am Maarten Lankhorst wrote:
Hi Chris,
2008/3/14, Chris Robinson chris.kcat@gmail.com:
On Thursday 13 March 2008 09:01:53 pm Maarten Lankhorst wrote:
If read from a file source, ID3 v2 tags might be incorrectly parsed by the head_check. This patch skips the whole tag altogether.
I'm not sure this is completely correct. The main splitter function detects the presense of an MP3 header at the start of a sample it's sending to set the sample's sync point attribute. Since the ID3 tag is not a sync point, this causes the first sample to not be set as a sync point, and it would be unlikely that a following sample would, under normal circumstances, contain an MP3 header at the start either. This could cause sensitive codecs to never find a sync point.
In general the tag is not a sync point, but some data behind it accidentally may be parsed as such. Even if it didn't, the tag is generally a few 100 (or about 1000 in my case) bytes in size. Reading all those bytes individually and moving them all 4 times doesn't seem really optimal. Skipping the amount of data that is set in the size buffer causes it to immediately find the first mp3 header.
Correct, but the patch only handles when the pin connection happens. The main splitter function will go through the start of the data again, and find the ID3 tag again, where it doesn't find the header to set the sync point (unlike the pin-connection, it doesn't continue to search forward and will just send it as is without the sync point attribute, though).
The following filter (acmwrapper or a native one) will also get the ID3 tag and will have to be able to handle it, otherwise it'll fail just like the splitter would.
Hi Chris,
2008/3/14, Chris Robinson chris.kcat@gmail.com:
On Friday 14 March 2008 10:29:34 am Maarten Lankhorst wrote:
Hi Chris,
2008/3/14, Chris Robinson chris.kcat@gmail.com:
On Thursday 13 March 2008 09:01:53 pm Maarten Lankhorst wrote:
If read from a file source, ID3 v2 tags might be incorrectly parsed by the head_check. This patch skips the whole tag altogether.
I'm not sure this is completely correct. The main splitter function detects the presense of an MP3 header at the start of a sample it's sending to set the sample's sync point attribute. Since the ID3 tag is not a sync point, this causes the first sample to not be set as a sync point, and it would be unlikely that a following sample would, under normal circumstances, contain an MP3 header at the start either. This could cause sensitive codecs to never find a sync point.
In general the tag is not a sync point, but some data behind it accidentally may be parsed as such. Even if it didn't, the tag is generally a few 100 (or about 1000 in my case) bytes in size. Reading all those bytes individually and moving them all 4 times doesn't seem really optimal. Skipping the amount of data that is set in the size buffer causes it to immediately find the first mp3 header.
Correct, but the patch only handles when the pin connection happens. The main splitter function will go through the start of the data again, and find the ID3 tag again, where it doesn't find the header to set the sync point (unlike the pin-connection, it doesn't continue to search forward and will just send it as is without the sync point attribute, though).
Considering it's metadata that doesn't have any effect for the next stages, it shouldn't be sent upstream. It should just be dropped locally. I missed the main splitter function, I'll fix that one too.
Cheers, Maarten.
-
Chris Robinson -
Maarten Lankhorst