diff mbox series

[FFmpeg-devel,v2] avformat/pp_bnk: treat music files are stereo

Message ID 20210316083019.25686-1-zane@zanevaniperen.com
State Accepted
Headers show
Series [FFmpeg-devel,v2] avformat/pp_bnk: treat music files are stereo
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Zane van Iperen March 16, 2021, 8:30 a.m. UTC
These files are technically a series of planar mono tracks.
If the "music" flag is set, merge the packets from the two
mono tracks, essentially replicating:

  [0:a:0][0:a:1]join=inputs=2:channel_layout=stereo[a]

Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
---
 libavformat/pp_bnk.c | 51 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 6 deletions(-)

Comments

Andreas Rheinhardt March 16, 2021, 8:40 a.m. UTC | #1
Zane van Iperen:
> These files are technically a series of planar mono tracks.
> If the "music" flag is set, merge the packets from the two
> mono tracks, essentially replicating:
> 
>   [0:a:0][0:a:1]join=inputs=2:channel_layout=stereo[a]
> 
> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
> ---
>  libavformat/pp_bnk.c | 51 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/pp_bnk.c b/libavformat/pp_bnk.c
> index 8364de1fd9..eba53a01a3 100644
> --- a/libavformat/pp_bnk.c
> +++ b/libavformat/pp_bnk.c
> @@ -55,6 +55,7 @@ typedef struct PPBnkCtx {
>      int             track_count;
>      PPBnkCtxTrack   *tracks;
>      uint32_t        current_track;
> +    int             is_music;
>  } PPBnkCtx;
>  
>  enum {
> @@ -194,8 +195,12 @@ static int pp_bnk_read_header(AVFormatContext *s)
>          goto fail;
>      }
>  
> +    ctx->is_music = (hdr.flags & PP_BNK_FLAG_MUSIC) &&
> +                    (ctx->track_count == 2) &&
> +                    (ctx->tracks[0].data_size == ctx->tracks[1].data_size);
> +
>      /* Build the streams. */
> -    for (int i = 0; i < ctx->track_count; i++) {
> +    for (int i = 0; i < (ctx->is_music ? 1 : ctx->track_count); i++) {
>          if (!(st = avformat_new_stream(s, NULL))) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
> @@ -204,14 +209,21 @@ static int pp_bnk_read_header(AVFormatContext *s)
>          par                         = st->codecpar;
>          par->codec_type             = AVMEDIA_TYPE_AUDIO;
>          par->codec_id               = AV_CODEC_ID_ADPCM_IMA_CUNNING;
> -        par->format                 = AV_SAMPLE_FMT_S16;
> -        par->channel_layout         = AV_CH_LAYOUT_MONO;
> -        par->channels               = 1;
> +        par->format                 = AV_SAMPLE_FMT_S16P;
> +
> +        if (ctx->is_music) {
> +            par->channel_layout     = AV_CH_LAYOUT_STEREO;
> +            par->channels           = 2;
> +        } else {
> +            par->channel_layout     = AV_CH_LAYOUT_MONO;
> +            par->channels           = 1;
> +        }
> +
>          par->sample_rate            = hdr.sample_rate;
>          par->bits_per_coded_sample  = 4;
>          par->bits_per_raw_sample    = 16;
>          par->block_align            = 1;
> -        par->bit_rate               = par->sample_rate * par->bits_per_coded_sample;
> +        par->bit_rate               = par->sample_rate * par->bits_per_coded_sample * par->channels;
>  
>          avpriv_set_pts_info(st, 64, 1, par->sample_rate);
>          st->start_time              = 0;
> @@ -253,7 +265,22 @@ static int pp_bnk_read_packet(AVFormatContext *s, AVPacket *pkt)
>  
>          size = FFMIN(trk->data_size - trk->bytes_read, PP_BNK_MAX_READ_SIZE);
>  
> -        if ((ret = av_get_packet(s->pb, pkt, size)) == AVERROR_EOF) {
> +        if (!ctx->is_music)
> +            ret = av_new_packet(pkt, size);
> +        else if (ctx->current_track == 0)
> +            ret = av_new_packet(pkt, size * 2);
> +        else
> +            ret = 0;
> +
> +        if (ret < 0)
> +            return ret;
> +
> +        if (ctx->is_music)
> +            ret = avio_read(s->pb, pkt->data + size * ctx->current_track, size);
> +        else
> +            ret = avio_read(s->pb, pkt->data, size);
> +
> +        if (ret == AVERROR_EOF) {
>              /* If we've hit EOF, don't attempt this track again. */
>              trk->data_size = trk->bytes_read;
>              continue;
> @@ -265,6 +292,18 @@ static int pp_bnk_read_packet(AVFormatContext *s, AVPacket *pkt)
>          pkt->flags         &= ~AV_PKT_FLAG_CORRUPT;
>          pkt->stream_index   = ctx->current_track++;
>          pkt->duration       = ret * 2;
> +
> +        if (ctx->is_music) {
> +            if (pkt->stream_index == 0) {
> +                ctx->current_track--;

I have to admit to be confused by this. Won't this imply that
ctx->current_track will always be zero for music files until you hit the
bytes_read == data_size check and that you just overwrite and therefore
leak the already allocated packets?

> +                continue;
> +            }
> +
> +            pkt->stream_index = 0;
> +        } else {
> +            pkt->size = ret;
> +        }
> +
>          return 0;
>      }
>  
>
Zane van Iperen March 16, 2021, 8:47 a.m. UTC | #2
On 16/3/21 6:40 pm, Andreas Rheinhardt wrote:

>> +
>> +        if (ctx->is_music) {
>> +            if (pkt->stream_index == 0) {
>> +                ctx->current_track--;
> 
> I have to admit to be confused by this. Won't this imply that
> ctx->current_track will always be zero for music files until you hit the
> bytes_read == data_size check and that you just overwrite and therefore
> leak the already allocated packets?
> 

No, because it's a continue instead of a return, I need to counteract the
"ctx->current_track++" in the above for(;;).
Zane van Iperen March 16, 2021, 8:50 a.m. UTC | #3
On 16/3/21 6:47 pm, Zane van Iperen wrote:
> 
> 
> On 16/3/21 6:40 pm, Andreas Rheinhardt wrote:
> 
>>> +
>>> +        if (ctx->is_music) {
>>> +            if (pkt->stream_index == 0) {
>>> +                ctx->current_track--;
>>
>> I have to admit to be confused by this. Won't this imply that
>> ctx->current_track will always be zero for music files until you hit the
>> bytes_read == data_size check and that you just overwrite and therefore
>> leak the already allocated packets?
>>
> 
> No, because it's a continue instead of a return, I need to counteract the
> "ctx->current_track++" in the above for(;;).

Forgot to mention, it's also incremented immediately above:

   pkt->stream_index   = ctx->current_track++;

...Yeah, I might clean this up a bit.
Andreas Rheinhardt March 16, 2021, 8:52 a.m. UTC | #4
Zane van Iperen:
> 
> 
> On 16/3/21 6:40 pm, Andreas Rheinhardt wrote:
> 
>>> +
>>> +        if (ctx->is_music) {
>>> +            if (pkt->stream_index == 0) {
>>> +                ctx->current_track--;
>>
>> I have to admit to be confused by this. Won't this imply that
>> ctx->current_track will always be zero for music files until you hit the
>> bytes_read == data_size check and that you just overwrite and therefore
>> leak the already allocated packets?
>>
> 
> No, because it's a continue instead of a return, I need to counteract the
> "ctx->current_track++" in the above for(;;).

I didn't think about that; I only saw pkt->stream_index   =
ctx->current_track++;.

- Andreas
Moritz Barsnick March 16, 2021, 8:56 a.m. UTC | #5
On Tue, Mar 16, 2021 at 18:30:19 +1000, Zane van Iperen wrote:

Nit:

> Subject: avformat/pp_bnk: treat music files are stereo
                                              ^ as

Moritz
diff mbox series

Patch

diff --git a/libavformat/pp_bnk.c b/libavformat/pp_bnk.c
index 8364de1fd9..eba53a01a3 100644
--- a/libavformat/pp_bnk.c
+++ b/libavformat/pp_bnk.c
@@ -55,6 +55,7 @@  typedef struct PPBnkCtx {
     int             track_count;
     PPBnkCtxTrack   *tracks;
     uint32_t        current_track;
+    int             is_music;
 } PPBnkCtx;
 
 enum {
@@ -194,8 +195,12 @@  static int pp_bnk_read_header(AVFormatContext *s)
         goto fail;
     }
 
+    ctx->is_music = (hdr.flags & PP_BNK_FLAG_MUSIC) &&
+                    (ctx->track_count == 2) &&
+                    (ctx->tracks[0].data_size == ctx->tracks[1].data_size);
+
     /* Build the streams. */
-    for (int i = 0; i < ctx->track_count; i++) {
+    for (int i = 0; i < (ctx->is_music ? 1 : ctx->track_count); i++) {
         if (!(st = avformat_new_stream(s, NULL))) {
             ret = AVERROR(ENOMEM);
             goto fail;
@@ -204,14 +209,21 @@  static int pp_bnk_read_header(AVFormatContext *s)
         par                         = st->codecpar;
         par->codec_type             = AVMEDIA_TYPE_AUDIO;
         par->codec_id               = AV_CODEC_ID_ADPCM_IMA_CUNNING;
-        par->format                 = AV_SAMPLE_FMT_S16;
-        par->channel_layout         = AV_CH_LAYOUT_MONO;
-        par->channels               = 1;
+        par->format                 = AV_SAMPLE_FMT_S16P;
+
+        if (ctx->is_music) {
+            par->channel_layout     = AV_CH_LAYOUT_STEREO;
+            par->channels           = 2;
+        } else {
+            par->channel_layout     = AV_CH_LAYOUT_MONO;
+            par->channels           = 1;
+        }
+
         par->sample_rate            = hdr.sample_rate;
         par->bits_per_coded_sample  = 4;
         par->bits_per_raw_sample    = 16;
         par->block_align            = 1;
-        par->bit_rate               = par->sample_rate * par->bits_per_coded_sample;
+        par->bit_rate               = par->sample_rate * par->bits_per_coded_sample * par->channels;
 
         avpriv_set_pts_info(st, 64, 1, par->sample_rate);
         st->start_time              = 0;
@@ -253,7 +265,22 @@  static int pp_bnk_read_packet(AVFormatContext *s, AVPacket *pkt)
 
         size = FFMIN(trk->data_size - trk->bytes_read, PP_BNK_MAX_READ_SIZE);
 
-        if ((ret = av_get_packet(s->pb, pkt, size)) == AVERROR_EOF) {
+        if (!ctx->is_music)
+            ret = av_new_packet(pkt, size);
+        else if (ctx->current_track == 0)
+            ret = av_new_packet(pkt, size * 2);
+        else
+            ret = 0;
+
+        if (ret < 0)
+            return ret;
+
+        if (ctx->is_music)
+            ret = avio_read(s->pb, pkt->data + size * ctx->current_track, size);
+        else
+            ret = avio_read(s->pb, pkt->data, size);
+
+        if (ret == AVERROR_EOF) {
             /* If we've hit EOF, don't attempt this track again. */
             trk->data_size = trk->bytes_read;
             continue;
@@ -265,6 +292,18 @@  static int pp_bnk_read_packet(AVFormatContext *s, AVPacket *pkt)
         pkt->flags         &= ~AV_PKT_FLAG_CORRUPT;
         pkt->stream_index   = ctx->current_track++;
         pkt->duration       = ret * 2;
+
+        if (ctx->is_music) {
+            if (pkt->stream_index == 0) {
+                ctx->current_track--;
+                continue;
+            }
+
+            pkt->stream_index = 0;
+        } else {
+            pkt->size = ret;
+        }
+
         return 0;
     }