[FFmpeg-devel] [PATCH] ffmpeg doesn't decode track number on some wma files

[Patrice Bensoussan]

On 14 Jun 2007, at 09:52, Michael Niedermayer wrote:

> Hi
>
> On Thu, Jun 14, 2007 at 09:13:52AM +0100, Patrice Bensoussan wrote:
>>
>> On 14 Jun 2007, at 09:00, Benoit Fouet wrote:
>>
>>> Patrice Bensoussan wrote:
>>>> Hello,
>>>>
>>>> On 14 Jun 2007, at 08:40, Benoit Fouet wrote:
>>>>
>>>>
>>>>> Hi,
>>>>>
>>>>> Patrice Bensoussan wrote:
>>>>>
>>>>>> Index: libavformat/asf.c
>>>>>> ================================================================= 
>>>>>> ==
>>>>>> --- libavformat/asf.c	(revision 9304)
>>>>>> +++ libavformat/asf.c	(working copy)
>>>>>> @@ -389,6 +389,16 @@
>>>>>>                         {
>>>>>>                                 if     (!strcmp(name,"WM/
>>>>>> AlbumTitle")) get_str16_nolen(pb, value_len, s->album, sizeof(s-
>>>>>>
>>>>>>> album));
>>>>>>>
>>>>>>                                 else if(!strcmp(name,"WM/
>>>>>> Genre"     )) get_str16_nolen(pb, value_len, s->genre, sizeof(s-
>>>>>>
>>>>>>> genre));
>>>>>>>
>>>>>> +                                else if (!strcmp(name,"WM/
>>>>>> Track")) {
>>>>>> +                                    char track[8];
>>>>>> +                                    get_str16_nolen(pb,
>>>>>> value_len, track, sizeof(track));
>>>>>> +                                    s->track = strtol(track,
>>>>>> NULL, 10) + 1;
>>>>>> +                                }
>>>>>> +                                else if (!strcmp(name,"WM/
>>>>>> TrackNumber")) {
>>>>>> +                                    char track[8];
>>>>>> +                                    get_str16_nolen(pb,
>>>>>> value_len, track, sizeof(track));
>>>>>> +                                    s->track = strtol(track,
>>>>>> NULL, 10);
>>>>>> +                                }
>>>>>>
>>>>>>
>>>>> you should try to keep the nice alignment
>>>>> also, if i read correctly, it is the very same code for the two
>>>>> else if
>>>>> you added. Why not add them into a single one ?
>>>>>
>>>>
>>>> The alignment seems to be correct for me (I checked with an hex
>>>> editor and can only see spaces unless I am missing something).
>>>
>>> i was more thinking of if and strcmp's parenthesis alignment
>>>
>>
>> OK, I missed that one. Here is a new patch with the if parenthesis
>> aligned.
> [...]
>> Index: libavformat/asf.c
>> ===================================================================
>> --- libavformat/asf.c	(revision 9304)
>> +++ libavformat/asf.c	(working copy)
>> @@ -389,6 +389,16 @@
>>                          {
>>                                  if     (!strcmp(name,"WM/ 
>> AlbumTitle")) get_str16_nolen(pb, value_len, s->album, sizeof(s- 
>> >album));
>>                                  else if(!strcmp(name,"WM/ 
>> Genre"     )) get_str16_nolen(pb, value_len, s->genre, sizeof(s- 
>> >genre));
>> +                                else if(!strcmp(name,"WM/Track")) {
>> +                                    char track[8];
>> +                                    get_str16_nolen(pb,  
>> value_len, track, sizeof(track));
>> +                                    s->track = strtol(track,  
>> NULL, 10) + 1;
>> +                                }
>> +                                else if(!strcmp(name,"WM/ 
>> TrackNumber")) {
>> +                                    char track[8];
>> +                                    get_str16_nolen(pb,  
>> value_len, track, sizeof(track));
>> +                                    s->track = strtol(track,  
>> NULL, 10);
>> +                                }
>
> what about
> char buf[123];
> get_str16_nolen(pb, value_len, track, sizeof(buf));
> if(     !strcmp(name,"WM/AlbumTitle")) copy
> else if(!strcmp(name,"WM/Genre"     )) copy
> else if(!strcmp(name,"WM/Track"     )) s->track = strtol(track,  
> NULL, 10) + 1;
> ...
>
> ?

This code would be slower (you are going to do some extra copying and  
I think it is less readable IMHO. I would rather stick with the patch  
submitted.

Patrice