diff mbox series

[FFmpeg-devel] avformat/ty: Remove write-only array and variable

Message ID 20200811012659.20934-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 7ad4928580dc570af156094160dc4c640d31a288
Headers show
Series [FFmpeg-devel] avformat/ty: Remove write-only array and variable
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 11, 2020, 1:26 a.m. UTC
Up until now, the TiVo demuxer parse an array of SEQ entries, yet it has
never ever made any use of them. In fact, parse_master, the function
parsing said table, only influenced the outside world in three ways: Via
an excessive amount of error message in case a certain parameter is not
what it expected; via an allocation (the aforementioned write-only
array); and by setting a certain parameter (ty->cur_chunk_pos), but that
parameter is always overwritten before it is used (it is overwritten
in get_chunk() on success and if get_chunk() fails, the error is
returned to the caller anyway). So remove the array and the function
used to parse it.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Furthermore, the parsing code seems not to work at all: The current
position is not incremented by map_size if map_size <= 8.

 libavformat/ty.c | 68 +-----------------------------------------------
 1 file changed, 1 insertion(+), 67 deletions(-)

Comments

Paul B Mahol Aug. 11, 2020, 9:58 a.m. UTC | #1
Make sure this does not break existing files,

On 8/11/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> Up until now, the TiVo demuxer parse an array of SEQ entries, yet it has
> never ever made any use of them. In fact, parse_master, the function
> parsing said table, only influenced the outside world in three ways: Via
> an excessive amount of error message in case a certain parameter is not
> what it expected; via an allocation (the aforementioned write-only
> array); and by setting a certain parameter (ty->cur_chunk_pos), but that
> parameter is always overwritten before it is used (it is overwritten
> in get_chunk() on success and if get_chunk() fails, the error is
> returned to the caller anyway). So remove the array and the function
> used to parse it.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Furthermore, the parsing code seems not to work at all: The current
> position is not incremented by map_size if map_size <= 8.
>
>  libavformat/ty.c | 68 +-----------------------------------------------
>  1 file changed, 1 insertion(+), 67 deletions(-)
>
> diff --git a/libavformat/ty.c b/libavformat/ty.c
> index 738a22e7de..c8e1067c0e 100644
> --- a/libavformat/ty.c
> +++ b/libavformat/ty.c
> @@ -72,11 +72,6 @@ typedef enum {
>      TIVO_AUDIO_MPEG
>  } TiVo_audio;
>
> -typedef struct TySeqTable {
> -    uint64_t    timestamp;
> -    uint8_t     chunk_bitmask[8];
> -} TySeqTable;
> -
>  typedef struct TYDemuxContext {
>      unsigned        cur_chunk;
>      unsigned        cur_chunk_pos;
> @@ -90,7 +85,6 @@ typedef struct TYDemuxContext {
>      int             pes_buf_cnt;      /* how many bytes in our buffer */
>      size_t          ac3_pkt_size;     /* length of ac3 pkt we've seen so
> far */
>      uint64_t        last_ty_pts;      /* last TY timestamp we've seen */
> -    unsigned        seq_table_size;   /* number of entries in SEQ table */
>
>      int64_t         first_audio_pts;
>      int64_t         last_audio_pts;
> @@ -99,8 +93,6 @@ typedef struct TYDemuxContext {
>      TyRecHdr       *rec_hdrs;         /* record headers array */
>      int             cur_rec;          /* current record in this chunk */
>      int             num_recs;         /* number of recs in this chunk */
> -    int             seq_rec;          /* record number where seq start is
> */
> -    TySeqTable     *seq_table;        /* table of SEQ entries from mstr chk
> */
>      int             first_chunk;
>
>      uint8_t         chunk[CHUNK_SIZE];
> @@ -339,58 +331,6 @@ static int ty_read_header(AVFormatContext *s)
>      return 0;
>  }
>
> -/* parse a master chunk, filling the SEQ table and other variables.
> - * We assume the stream is currently pointing to it.
> - */
> -static void parse_master(AVFormatContext *s)
> -{
> -    TYDemuxContext *ty = s->priv_data;
> -    unsigned map_size;  /* size of bitmask, in bytes */
> -    unsigned i, j;
> -
> -    /* Note that the entries in the SEQ table in the stream may have
> -       different sizes depending on the bits per entry.  We store them
> -       all in the same size structure, so we have to parse them out one
> -       by one.  If we had a dynamic structure, we could simply read the
> -       entire table directly from the stream into memory in place. */
> -
> -    /* clear the SEQ table */
> -    av_freep(&ty->seq_table);
> -
> -    /* parse header info */
> -
> -    map_size = AV_RB32(ty->chunk + 20);  /* size of bitmask, in bytes */
> -    i = AV_RB32(ty->chunk + 28);   /* size of SEQ table, in bytes */
> -
> -    ty->seq_table_size = i / (8LL + map_size);
> -
> -    if (ty->seq_table_size == 0) {
> -        ty->seq_table = NULL;
> -        return;
> -    }
> -
> -    /* parse all the entries */
> -    ty->seq_table = av_calloc(ty->seq_table_size, sizeof(TySeqTable));
> -    if (ty->seq_table == NULL) {
> -        ty->seq_table_size = 0;
> -        return;
> -    }
> -
> -    ty->cur_chunk_pos = 32;
> -    for (j = 0; j < ty->seq_table_size; j++) {
> -        if (ty->cur_chunk_pos >= CHUNK_SIZE - 8)
> -            return;
> -        ty->seq_table[j].timestamp = AV_RB64(ty->chunk +
> ty->cur_chunk_pos);
> -        ty->cur_chunk_pos += 8;
> -        if (map_size > 8) {
> -            av_log(s, AV_LOG_ERROR, "Unsupported SEQ bitmap size in master
> chunk.\n");
> -            ty->cur_chunk_pos += map_size;
> -        } else {
> -            memcpy(ty->seq_table[j].chunk_bitmask, ty->chunk +
> ty->cur_chunk_pos, map_size);
> -        }
> -    }
> -}
> -
>  static int get_chunk(AVFormatContext *s)
>  {
>      TYDemuxContext *ty = s->priv_data;
> @@ -413,7 +353,7 @@ static int get_chunk(AVFormatContext *s)
>
>      /* check if it's a PART Header */
>      if (AV_RB32(ty->chunk) == TIVO_PES_FILEID) {
> -        parse_master(s); /* parse master chunk */
> +        /* skip master chunk and read new chunk */
>          return get_chunk(s);
>      }
>
> @@ -421,14 +361,9 @@ static int get_chunk(AVFormatContext *s)
>      if (ty->chunk[3] & 0x80) {
>          /* 16 bit rec cnt */
>          ty->num_recs = num_recs = (ty->chunk[1] << 8) + ty->chunk[0];
> -        ty->seq_rec = (ty->chunk[3] << 8) + ty->chunk[2];
> -        if (ty->seq_rec != 0xffff) {
> -            ty->seq_rec &= ~0x8000;
> -        }
>      } else {
>          /* 8 bit reclen - TiVo 1.3 format */
>          ty->num_recs = num_recs = ty->chunk[0];
> -        ty->seq_rec = ty->chunk[1];
>      }
>      ty->cur_rec = 0;
>      ty->first_chunk = 0;
> @@ -770,7 +705,6 @@ static int ty_read_close(AVFormatContext *s)
>  {
>      TYDemuxContext *ty = s->priv_data;
>
> -    av_freep(&ty->seq_table);
>      av_freep(&ty->rec_hdrs);
>
>      return 0;
> --
> 2.20.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Aug. 11, 2020, 12:40 p.m. UTC | #2
Paul B Mahol:
> Make sure this does not break existing files,
> 

I tested with all the samples in https://samples.ffmpeg.org/TiVo/; of
course, there was no change in output at all. So I'll apply this.

- Andreas

> On 8/11/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>> Up until now, the TiVo demuxer parse an array of SEQ entries, yet it has
>> never ever made any use of them. In fact, parse_master, the function
>> parsing said table, only influenced the outside world in three ways: Via
>> an excessive amount of error message in case a certain parameter is not
>> what it expected; via an allocation (the aforementioned write-only
>> array); and by setting a certain parameter (ty->cur_chunk_pos), but that
>> parameter is always overwritten before it is used (it is overwritten
>> in get_chunk() on success and if get_chunk() fails, the error is
>> returned to the caller anyway). So remove the array and the function
>> used to parse it.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> Furthermore, the parsing code seems not to work at all: The current
>> position is not incremented by map_size if map_size <= 8.
>>
>>  libavformat/ty.c | 68 +-----------------------------------------------
>>  1 file changed, 1 insertion(+), 67 deletions(-)
diff mbox series

Patch

diff --git a/libavformat/ty.c b/libavformat/ty.c
index 738a22e7de..c8e1067c0e 100644
--- a/libavformat/ty.c
+++ b/libavformat/ty.c
@@ -72,11 +72,6 @@  typedef enum {
     TIVO_AUDIO_MPEG
 } TiVo_audio;
 
-typedef struct TySeqTable {
-    uint64_t    timestamp;
-    uint8_t     chunk_bitmask[8];
-} TySeqTable;
-
 typedef struct TYDemuxContext {
     unsigned        cur_chunk;
     unsigned        cur_chunk_pos;
@@ -90,7 +85,6 @@  typedef struct TYDemuxContext {
     int             pes_buf_cnt;      /* how many bytes in our buffer */
     size_t          ac3_pkt_size;     /* length of ac3 pkt we've seen so far */
     uint64_t        last_ty_pts;      /* last TY timestamp we've seen */
-    unsigned        seq_table_size;   /* number of entries in SEQ table */
 
     int64_t         first_audio_pts;
     int64_t         last_audio_pts;
@@ -99,8 +93,6 @@  typedef struct TYDemuxContext {
     TyRecHdr       *rec_hdrs;         /* record headers array */
     int             cur_rec;          /* current record in this chunk */
     int             num_recs;         /* number of recs in this chunk */
-    int             seq_rec;          /* record number where seq start is */
-    TySeqTable     *seq_table;        /* table of SEQ entries from mstr chk */
     int             first_chunk;
 
     uint8_t         chunk[CHUNK_SIZE];
@@ -339,58 +331,6 @@  static int ty_read_header(AVFormatContext *s)
     return 0;
 }
 
-/* parse a master chunk, filling the SEQ table and other variables.
- * We assume the stream is currently pointing to it.
- */
-static void parse_master(AVFormatContext *s)
-{
-    TYDemuxContext *ty = s->priv_data;
-    unsigned map_size;  /* size of bitmask, in bytes */
-    unsigned i, j;
-
-    /* Note that the entries in the SEQ table in the stream may have
-       different sizes depending on the bits per entry.  We store them
-       all in the same size structure, so we have to parse them out one
-       by one.  If we had a dynamic structure, we could simply read the
-       entire table directly from the stream into memory in place. */
-
-    /* clear the SEQ table */
-    av_freep(&ty->seq_table);
-
-    /* parse header info */
-
-    map_size = AV_RB32(ty->chunk + 20);  /* size of bitmask, in bytes */
-    i = AV_RB32(ty->chunk + 28);   /* size of SEQ table, in bytes */
-
-    ty->seq_table_size = i / (8LL + map_size);
-
-    if (ty->seq_table_size == 0) {
-        ty->seq_table = NULL;
-        return;
-    }
-
-    /* parse all the entries */
-    ty->seq_table = av_calloc(ty->seq_table_size, sizeof(TySeqTable));
-    if (ty->seq_table == NULL) {
-        ty->seq_table_size = 0;
-        return;
-    }
-
-    ty->cur_chunk_pos = 32;
-    for (j = 0; j < ty->seq_table_size; j++) {
-        if (ty->cur_chunk_pos >= CHUNK_SIZE - 8)
-            return;
-        ty->seq_table[j].timestamp = AV_RB64(ty->chunk + ty->cur_chunk_pos);
-        ty->cur_chunk_pos += 8;
-        if (map_size > 8) {
-            av_log(s, AV_LOG_ERROR, "Unsupported SEQ bitmap size in master chunk.\n");
-            ty->cur_chunk_pos += map_size;
-        } else {
-            memcpy(ty->seq_table[j].chunk_bitmask, ty->chunk + ty->cur_chunk_pos, map_size);
-        }
-    }
-}
-
 static int get_chunk(AVFormatContext *s)
 {
     TYDemuxContext *ty = s->priv_data;
@@ -413,7 +353,7 @@  static int get_chunk(AVFormatContext *s)
 
     /* check if it's a PART Header */
     if (AV_RB32(ty->chunk) == TIVO_PES_FILEID) {
-        parse_master(s); /* parse master chunk */
+        /* skip master chunk and read new chunk */
         return get_chunk(s);
     }
 
@@ -421,14 +361,9 @@  static int get_chunk(AVFormatContext *s)
     if (ty->chunk[3] & 0x80) {
         /* 16 bit rec cnt */
         ty->num_recs = num_recs = (ty->chunk[1] << 8) + ty->chunk[0];
-        ty->seq_rec = (ty->chunk[3] << 8) + ty->chunk[2];
-        if (ty->seq_rec != 0xffff) {
-            ty->seq_rec &= ~0x8000;
-        }
     } else {
         /* 8 bit reclen - TiVo 1.3 format */
         ty->num_recs = num_recs = ty->chunk[0];
-        ty->seq_rec = ty->chunk[1];
     }
     ty->cur_rec = 0;
     ty->first_chunk = 0;
@@ -770,7 +705,6 @@  static int ty_read_close(AVFormatContext *s)
 {
     TYDemuxContext *ty = s->priv_data;
 
-    av_freep(&ty->seq_table);
     av_freep(&ty->rec_hdrs);
 
     return 0;