[vlc-devel] [PATCH v2] demux: wav: support parsing INFO chunk
Zhao, Gang
gang.zhao.42 at gmail.com
Tue Sep 15 09:25:59 CEST 2020
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.
>
> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200915/944d8eeb/attachment.html>
More information about the vlc-devel
mailing list