[vlc-devel] [PATCH v2] demux: wav: support parsing INFO chunk
Thomas Guillem
thomas at gllm.fr
Tue Sep 15 10:56:27 CEST 2020
On Tue, Sep 15, 2020, at 09:25, Zhao, Gang wrote:
> On Mon, Sep 14, 2020 at 6:54 PM Thomas Guillem <thomas at gllm.fr> wrote:
>> Hello,
>>
> Thanks for your review.
>
>> On Sun, Sep 13, 2020, at 04:20, Zhao, Gang wrote:
>> > Parse INFO chunk and save meta data to vlc.
>> >
>> > Feature ticket: https://trac.videolan.org/vlc/ticket/6587
>> >
>> > Signed-off-by: Zhao, Gang <gang.zhao.42 at gmail.com>
>> > ---
>> > Tested this patch with the four wav files in ticket 6587. It can get and save
>> > meta data from INFO chunk.
>> >
>> > v2:
>> > Fixed a typo in commit log
>> >
>> > modules/demux/wav.c | 216 ++++++++++++++++++++++++++++++++++++++++++--
>> > 1 file changed, 207 insertions(+), 9 deletions(-)
>> >
>> > diff --git modules/demux/wav.c modules/demux/wav.c
>> > index 4b0623412f..75b37d5be0 100644
>> > --- modules/demux/wav.c
>> > +++ modules/demux/wav.c
>> > @@ -36,6 +36,8 @@
>> > #include <vlc_demux.h>
>> > #include <vlc_aout.h>
>> > #include <vlc_codecs.h>
>> > +#include <vlc_charset.h>
>> > +#include <vlc_meta.h>
>> >
>> > #include "windows_audio_commons.h"
>> >
>> > @@ -58,26 +60,96 @@ typedef struct
>> > uint32_t i_channel_mask;
>> > uint8_t i_chans_to_reorder; /* do we need channel reordering */
>> > uint8_t pi_chan_table[AOUT_CHAN_MAX];
>> > +
>> > + vlc_meta_t *p_meta;
>> > } demux_sys_t;
>> >
>> > enum wav_chunk_id {
>> > + wav_chunk_id_list,
>> > wav_chunk_id_data,
>> > wav_chunk_id_ds64,
>> > wav_chunk_id_fmt,
>> > +
>> > + wav_info_chunk_id_iarl,
>> > + wav_info_chunk_id_iart,
>> > + wav_info_chunk_id_icms,
>> > + wav_info_chunk_id_icmt,
>> > + wav_info_chunk_id_icop,
>> > + wav_info_chunk_id_icrd,
>> > + wav_info_chunk_id_icrp,
>> > + wav_info_chunk_id_idim,
>> > + wav_info_chunk_id_idpi,
>> > + wav_info_chunk_id_ieng,
>> > + wav_info_chunk_id_ignr,
>> > + wav_info_chunk_id_ikey,
>> > + wav_info_chunk_id_ilgt,
>> > + wav_info_chunk_id_imed,
>> > + wav_info_chunk_id_inam,
>> > + wav_info_chunk_id_iplt,
>> > + wav_info_chunk_id_iprd,
>> > + wav_info_chunk_id_isbj,
>> > + wav_info_chunk_id_isft,
>> > + wav_info_chunk_id_ishp,
>> > + wav_info_chunk_id_isrc,
>> > + wav_info_chunk_id_isrf,
>> > + wav_info_chunk_id_itch,
>> > };
>> >
>> > -static const struct wav_chunk_id_key
>> > +struct wav_chunk_id_key
>> > {
>> > enum wav_chunk_id id;
>> > char key[5];
>> > -} wav_chunk_id_key_list[] = {
>> > +};
>> > +
>> > +static const struct wav_chunk_id_key wav_chunk_id_key_list[] = {
>> > /* Alphabetical order */
>> > + { wav_chunk_id_list, "LIST" },
>> > { wav_chunk_id_data, "data" },
>> > { wav_chunk_id_ds64, "ds64" },
>> > { wav_chunk_id_fmt, "fmt " },
>> > };
>> > static const size_t wav_chunk_id_key_count = ARRAY_SIZE(wav_chunk_id_key_list);
>> >
>> > +static const struct wav_chunk_id_key wav_info_chunk_id_key_list[] = {
>> > + /* Alphabetical order */
>> > + { wav_info_chunk_id_iarl, "IARL" },
>> > + { wav_info_chunk_id_iart, "IART" },
>> > + { wav_info_chunk_id_icms, "ICMS" },
>> > + { wav_info_chunk_id_icmt, "ICMT" },
>> > + { wav_info_chunk_id_icop, "ICOP" },
>> > + { wav_info_chunk_id_icrd, "ICRD" },
>> > + { wav_info_chunk_id_icrp, "ICRP" },
>> > + { wav_info_chunk_id_idim, "IDIM" },
>> > + { wav_info_chunk_id_idpi, "IDPI" },
>> > + { wav_info_chunk_id_ieng, "IENG" },
>> > + { wav_info_chunk_id_ignr, "IGNR" },
>> > + { wav_info_chunk_id_ikey, "IKEY" },
>> > + { wav_info_chunk_id_ilgt, "ILGT" },
>> > + { wav_info_chunk_id_imed, "IMED" },
>> > + { wav_info_chunk_id_inam, "INAM" },
>> > + { wav_info_chunk_id_iplt, "IPLT" },
>> > + { wav_info_chunk_id_iprd, "IPRD" },
>> > + { wav_info_chunk_id_isbj, "ISBJ" },
>> > + { wav_info_chunk_id_isft, "ISFT" },
>> > + { wav_info_chunk_id_ishp, "ISHP" },
>> > + { wav_info_chunk_id_isrc, "ISRC" },
>> > + { wav_info_chunk_id_isrf, "ISRF" },
>> > + { wav_info_chunk_id_itch, "ITCH" },
>> > +};
>> > +static const size_t wav_info_chunk_id_key_count =
>> > ARRAY_SIZE(wav_info_chunk_id_key_list);
>> > +
>> > +static const char *wav_info_chunk_id_to_key(enum wav_chunk_id id)
>> > +{
>> > + if ( id < wav_info_chunk_id_iarl || id > wav_info_chunk_id_itch )
>> > + return "";
>>
>> Your fonction is taking an enum in argument but is checking if the enum is out of bound. This seems weird.
>> You could check the validity of the id after ChunkGetNext() in ChunkParseINFO().
>>
> This function is used mostly for debug purpose. It'll print the relevant string representation of an enum.
>
> Ids like wav_chunk_id_list, their relevant string can be easily got as wav_chunk_id_key_list[wav_chunk_id_list].
> But ids like wav_info_chunk_id_iarl, their relevant string can't be got as wav_info_chunk_id_key_list[wav_info_chunk_id_iarl],
> because there's an offset to it. Function wav_info_chunk_id_to_key is a helper function to print the string representation of these ids.
>
> As this function is only useful for info chunk ids, it will check if the enum is in the range of info chunk ids.
>
>> > +
>> > + for ( int i = 0; i < wav_info_chunk_id_key_count; i++)
>> > + if (wav_info_chunk_id_key_list[i].id == id)
>> > + return wav_info_chunk_id_key_list[i].key;
>> > +
>> > + return "";
>> > +}
>> > +
>> > static int
>> > wav_chunk_CompareCb(const void *a, const void *b)
>> > {
>> > @@ -190,13 +262,14 @@ static int ChunkSkip( demux_t *p_demux, uint32_t
>> > i_size )
>> > }
>> >
>> > static int ChunkGetNext( demux_t *p_demux, enum wav_chunk_id *p_id,
>> > - uint32_t *pi_size )
>> > + uint32_t *pi_size,
>> > + const struct wav_chunk_id_key *id_key_list,
>> > size_t count )
>> > {
>> > #ifndef NDEBUG
>> > /* assert that keys are in alphabetical order */
>> > - for( size_t i = 0; i < wav_chunk_id_key_count - 1; ++i )
>> > - assert( strcmp( wav_chunk_id_key_list[i].key,
>> > - wav_chunk_id_key_list[i + 1].key ) < 0 );
>> > + for( size_t i = 0; i < count - 1; ++i )
>> > + assert( strcmp( id_key_list[i].key,
>> > + id_key_list[i + 1].key ) < 0 );
>> > #endif
>> >
>> > for( ;; )
>> > @@ -206,8 +279,8 @@ static int ChunkGetNext( demux_t *p_demux, enum
>> > wav_chunk_id *p_id,
>> > return VLC_EGENERIC;
>> >
>> > const struct wav_chunk_id_key *id =
>> > - bsearch( p_peek, wav_chunk_id_key_list, wav_chunk_id_key_count,
>> > - sizeof(*wav_chunk_id_key_list), wav_chunk_CompareCb );
>> > + bsearch( p_peek, id_key_list, count,
>> > + sizeof(*id_key_list), wav_chunk_CompareCb );
>> > uint32_t i_size = GetDWLE( p_peek + 4 );
>>
>> This could go in a separate function to improve readability.
>>
> I'm not sure what you meant here. The bsearch is needed to get the chunk id, which is an out parameter of
> function ChunkGetNext. I added two parameters to ChunkGetNext so it can search in different chunk id arrays.
> When we start parsing the wav file, we care about chunk ids in wav_chunk_id_key_list array. When we parsing INFO
> chunk, we care about chunk ids in wav_info_chunk_id_key_list array. My change made this two process can both use
> ChunkGetNext function to get chunk data.
Sorry, I wanted to say that it could go in a *separate* commit.
First commit: add change the ChunkGetNext() and adapt the whole code.
Second commit: add the INFO parsing.
>
>> >
>> > if( id == NULL )
>> > @@ -317,6 +390,10 @@ static void Close ( vlc_object_t * p_this )
>> > demux_sys_t *p_sys = p_demux->p_sys;
>> >
>> > es_format_Clean( &p_sys->fmt );
>> > +
>> > + if( p_sys->p_meta )
>> > + vlc_meta_Delete( p_sys->p_meta );
>> > +
>> > free( p_sys );
>> > }
>> >
>> > @@ -591,6 +668,114 @@ error:
>> > return VLC_EGENERIC;
>> > }
>> >
>> > +static int ChunkParseINFO( demux_t *p_demux, uint32_t i_size )
>> > +{
>> > + demux_sys_t *p_sys = p_demux->p_sys;
>> > + const uint8_t *p_peek;
>> > + enum wav_chunk_id id;
>> > + uint32_t chunk_size;
>> > + int meta_type;
>> > + char *meta_value;
>> > +
>> > + if( (p_sys->p_meta = vlc_meta_New()) == NULL )
>> > + {
>> > + msg_Err( p_demux, "vlc_meta_New failed" );
>> > + return VLC_EGENERIC;
>> > + }
>> > +
>> > + while (i_size > 0) {
>> > + int ret = ChunkGetNext( p_demux, &id, &chunk_size,
>> > + wav_info_chunk_id_key_list,
>> > + wav_info_chunk_id_key_count );
>> > + if ( ret != VLC_SUCCESS )
>> > + break;
>> > +
>> > + /* chunk id(4 bytes) and chunk size(4 bytes) consumed by
>> > ChunkGetNext */
>> > + i_size -= 8;
>> > +
>> > + if( chunk_size == 0 )
>> > + {
>> > + msg_Err( p_demux, "invalid chunk with a size 0" );
>> > + return VLC_EGENERIC;
>> > + }
>> > +
>> > + if( vlc_stream_Peek( p_demux->s, &p_peek, chunk_size ) !=
>> > chunk_size )
>> > + return VLC_EGENERIC;
>> > +
>> > + meta_value = strndup( (const char *)p_peek, chunk_size );
>> > + if (meta_value == NULL)
>> > + return VLC_EGENERIC;
>> > +
>> > + EnsureUTF8( meta_value );
>> > +
>> > + meta_type = -1;
>> > + switch( id )
>> > + {
>> > + case wav_info_chunk_id_iart:
>> > + meta_type = vlc_meta_Artist;
>> > + break;
>> > + case wav_info_chunk_id_icop:
>> > + meta_type = vlc_meta_Copyright;
>> > + break;
>> > + case wav_info_chunk_id_icrd:
>> > + meta_type = vlc_meta_Date;
>> > + break;
>> > + case wav_info_chunk_id_ignr:
>> > + meta_type = vlc_meta_Genre;
>> > + break;
>> > + case wav_info_chunk_id_inam:
>> > + meta_type = vlc_meta_Title;
>> > + break;
>> > + default:
>> > + /* no relevant meta type in vlc */
>> > + break;
>> > + }
>> > +
>> > + msg_Dbg( p_demux, "id: %s meta_type: %d meta_value: %s\n",
>> > + wav_info_chunk_id_to_key(id), meta_type, meta_value );
>> > +
>> > + if (meta_type != -1)
>> > + vlc_meta_Set( p_sys->p_meta, meta_type, meta_value );
>> > + else
>> > + vlc_meta_AddExtra( p_sys->p_meta,
>> > + wav_info_chunk_id_key_list[id].key,
>> > meta_value );
>> > +
>> > + free( meta_value );
>> > + if( ChunkSkip( p_demux, chunk_size ) != VLC_SUCCESS )
>> > + return VLC_EGENERIC;
>> > +
>> > + i_size -= chunk_size;
>> > + }
>> > +
>> > + return VLC_SUCCESS;
>> > +}
>> > +
>> > +static int ChunkParseLIST( demux_t *p_demux, uint32_t i_size )
>> > +{
>> > + const uint8_t *p_peek;
>> > +
>> > + /* must have four bytes list type */
>> > + if( i_size < 4 )
>> > + {
>> > + msg_Err( p_demux, "invalid 'LIST' chunk" );
>> > + return VLC_EGENERIC;
>> > + }
>> > +
>> > + if( vlc_stream_Peek( p_demux->s, &p_peek, 4 ) != 4 )
>> > + return VLC_EGENERIC;
>> > +
>> > + /* only care about INFO chunk */
>> > + if ( memcmp(p_peek, "INFO", 4) != 0 ) {
>> > + return ChunkSkip( p_demux, i_size );
>> > + }
>> > +
>> > + if ( vlc_stream_Read( p_demux->s, NULL, 4) != 4 )
>> > + return VLC_EGENERIC;
>> > +
>> > + i_size -= 4;
>> > + return ChunkParseINFO( p_demux, i_size );
>> > +}
>> > +
>> > static int Open( vlc_object_t * p_this )
>> > {
>> > demux_t *p_demux = (demux_t*)p_this;
>> > @@ -629,8 +814,13 @@ static int Open( vlc_object_t * p_this )
>> >
>> > bool eof = false;
>> > enum wav_chunk_id id;
>> > - while( !eof && ( ChunkGetNext( p_demux, &id, &i_size ) ) ==
>> > VLC_SUCCESS )
>> > + while( !eof )
>> > {
>> > + int ret = ChunkGetNext( p_demux, &id, &i_size,
>> > + wav_chunk_id_key_list,
>> > wav_chunk_id_key_count );
>> > + if (ret != VLC_SUCCESS )
>> > + break;
>> > +
>> > if( i_size == 0 )
>> > {
>> > msg_Err( p_demux, "invalid chunk with a size 0");
>> > @@ -676,6 +866,14 @@ static int Open( vlc_object_t * p_this )
>> > if( ChunkParseFmt( p_demux, i_size ) != VLC_SUCCESS )
>> > goto error;
>> > break;
>> > + case wav_chunk_id_list:
>> > + if( ChunkParseLIST( p_demux, i_size ) != VLC_SUCCESS )
>> > + goto error;
>> > + break;
>> > + default:
>> > + if( ChunkSkip( p_demux, i_size ) != VLC_SUCCESS )
>> > + goto error;
>> > + break;
>> > }
>> > }
>> >
>> > --
>> > 2.25.1
>> >
>> > _______________________________________________
>> > vlc-devel mailing list
>> > To unsubscribe or modify your subscription options:
>> > https://mailman.videolan.org/listinfo/vlc-devel
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200915/5fca7d43/attachment.html>
More information about the vlc-devel
mailing list