diff mbox series

[FFmpeg-devel,2/2] avformat/hnm: Only keep and parse what is needed later

Message ID 20200321063118.6790-2-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avformat/hnm: Check for extradata allocation failure
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 21, 2020, 6:31 a.m. UTC
The hnm demuxer's context struct contained lots of fields that are
write-only variables or that are not used outside of parsing the header
and that can therefore be replaced by local variables of hnm_read_header().
This commit removes all of these from the context; the second type has
been replaced by local variables.

An AVPacket (that was initialized when reading the header and for which
dead code to unreference it existed in hnm_read_close()) is among the
removed things. Removing it allowed to remove hnm_read_close()
altogether and also removes another instance of usage of sizeof(AVPacket).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/hnm.c | 63 +++++++++--------------------------------------
 1 file changed, 12 insertions(+), 51 deletions(-)

Comments

Paul B Mahol March 21, 2020, 9:24 a.m. UTC | #1
ok if fate passed and it is covered.

On 3/21/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> The hnm demuxer's context struct contained lots of fields that are
> write-only variables or that are not used outside of parsing the header
> and that can therefore be replaced by local variables of hnm_read_header().
> This commit removes all of these from the context; the second type has
> been replaced by local variables.
>
> An AVPacket (that was initialized when reading the header and for which
> dead code to unreference it existed in hnm_read_close()) is among the
> removed things. Removing it allowed to remove hnm_read_close()
> altogether and also removes another instance of usage of sizeof(AVPacket).
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/hnm.c | 63 +++++++++--------------------------------------
>  1 file changed, 12 insertions(+), 51 deletions(-)
>
> diff --git a/libavformat/hnm.c b/libavformat/hnm.c
> index 31221553a4..f06add5cf8 100644
> --- a/libavformat/hnm.c
> +++ b/libavformat/hnm.c
> @@ -37,19 +37,9 @@
>  #define HNM4_CHUNK_ID_SD 17491
>
>  typedef struct Hnm4DemuxContext {
> -    uint8_t version;
> -    uint16_t width;
> -    uint16_t height;
> -    uint32_t filesize;
>      uint32_t frames;
> -    uint32_t taboffset;
> -    uint16_t bits;
> -    uint16_t channels;
> -    uint32_t framesize;
>      uint32_t currentframe;
> -    int64_t pts;
>      uint32_t superchunk_remaining;
> -    AVPacket vpkt;
>  } Hnm4DemuxContext;
>
>  static int hnm_probe(const AVProbeData *p)
> @@ -69,55 +59,37 @@ static int hnm_read_header(AVFormatContext *s)
>  {
>      Hnm4DemuxContext *hnm = s->priv_data;
>      AVIOContext *pb = s->pb;
> +    unsigned width, height;
>      AVStream *vst;
>      int ret;
>
> -    /* default context members */
> -    hnm->pts = 0;
> -    av_init_packet(&hnm->vpkt);
> -    hnm->vpkt.data = NULL;
> -    hnm->vpkt.size = 0;
> -
> -    hnm->superchunk_remaining = 0;
> -
>      avio_skip(pb, 8);
> -    hnm->width     = avio_rl16(pb);
> -    hnm->height    = avio_rl16(pb);
> -    hnm->filesize  = avio_rl32(pb);
> +    width          = avio_rl16(pb);
> +    height         = avio_rl16(pb);
> +    avio_rl32(pb); // filesize
>      hnm->frames    = avio_rl32(pb);
> -    hnm->taboffset = avio_rl32(pb);
> -    hnm->bits      = avio_rl16(pb);
> -    hnm->channels  = avio_rl16(pb);
> -    hnm->framesize = avio_rl32(pb);
> -    avio_skip(pb, 32);
> +    avio_skip(pb, 44);
>
> -    hnm->currentframe = 0;
> -
> -    if (hnm->width  < 256 || hnm->width  > 640 ||
> -        hnm->height < 150 || hnm->height > 480) {
> +    if (width  < 256 || width  > 640 ||
> +        height < 150 || height > 480) {
>          av_log(s, AV_LOG_ERROR,
> -               "invalid resolution: %ux%u\n", hnm->width, hnm->height);
> +               "invalid resolution: %ux%u\n", width, height);
>          return AVERROR_INVALIDDATA;
>      }
>
> -    // TODO: find a better way to detect HNM4A
> -    if (hnm->width == 640)
> -        hnm->version = 0x4a;
> -    else
> -        hnm->version = 0x40;
> -
>      if (!(vst = avformat_new_stream(s, NULL)))
>          return AVERROR(ENOMEM);
>
>      vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>      vst->codecpar->codec_id   = AV_CODEC_ID_HNM4_VIDEO;
>      vst->codecpar->codec_tag  = 0;
> -    vst->codecpar->width      = hnm->width;
> -    vst->codecpar->height     = hnm->height;
> +    vst->codecpar->width      = width;
> +    vst->codecpar->height     = height;
>      if ((ret = ff_alloc_extradata(vst->codecpar, 1)) < 0)
>          return ret;
>
> -    vst->codecpar->extradata[0] = hnm->version;
> +    // TODO: find a better way to detect HNM4A
> +    vst->codecpar->extradata[0] = width == 640 ? 0x4a : 0x40;
>
>      vst->start_time = 0;
>
> @@ -186,16 +158,6 @@ static int hnm_read_packet(AVFormatContext *s, AVPacket
> *pkt)
>      return ret;
>  }
>
> -static int hnm_read_close(AVFormatContext *s)
> -{
> -    Hnm4DemuxContext *hnm = s->priv_data;
> -
> -    if (hnm->vpkt.size > 0)
> -        av_packet_unref(&hnm->vpkt);
> -
> -    return 0;
> -}
> -
>  AVInputFormat ff_hnm_demuxer = {
>      .name           = "hnm",
>      .long_name      = NULL_IF_CONFIG_SMALL("Cryo HNM v4"),
> @@ -203,6 +165,5 @@ AVInputFormat ff_hnm_demuxer = {
>      .read_probe     = hnm_probe,
>      .read_header    = hnm_read_header,
>      .read_packet    = hnm_read_packet,
> -    .read_close     = hnm_read_close,
>      .flags          = AVFMT_NO_BYTE_SEEK | AVFMT_NOGENSEARCH |
> AVFMT_NOBINSEARCH
>  };
> --
> 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 March 21, 2020, 3:16 p.m. UTC | #2
Paul B Mahol:
> ok if fate passed and it is covered.
> 
It is not covered, yet I checked all the samples from [1] as well as the
one from ticket #3464. (That the stuff I removed is actually unused can
also be seen from the fact that I did not modify hnm_read_packet() at all.)

- Andreas

[1]: https://samples.ffmpeg.org/game-formats/hnm/losteden-hnm4/
Andreas Rheinhardt April 3, 2020, 8:14 a.m. UTC | #3
Paul B Mahol:
> ok if fate passed and it is covered.
> 
> On 3/21/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>> The hnm demuxer's context struct contained lots of fields that are
>> write-only variables or that are not used outside of parsing the header
>> and that can therefore be replaced by local variables of hnm_read_header().
>> This commit removes all of these from the context; the second type has
>> been replaced by local variables.
>>
>> An AVPacket (that was initialized when reading the header and for which
>> dead code to unreference it existed in hnm_read_close()) is among the
>> removed things. Removing it allowed to remove hnm_read_close()
>> altogether and also removes another instance of usage of sizeof(AVPacket).
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavformat/hnm.c | 63 +++++++++--------------------------------------
>>  1 file changed, 12 insertions(+), 51 deletions(-)
>>
>> diff --git a/libavformat/hnm.c b/libavformat/hnm.c
>> index 31221553a4..f06add5cf8 100644
>> --- a/libavformat/hnm.c
>> +++ b/libavformat/hnm.c
>> @@ -37,19 +37,9 @@
>>  #define HNM4_CHUNK_ID_SD 17491
>>
>>  typedef struct Hnm4DemuxContext {
>> -    uint8_t version;
>> -    uint16_t width;
>> -    uint16_t height;
>> -    uint32_t filesize;
>>      uint32_t frames;
>> -    uint32_t taboffset;
>> -    uint16_t bits;
>> -    uint16_t channels;
>> -    uint32_t framesize;
>>      uint32_t currentframe;
>> -    int64_t pts;
>>      uint32_t superchunk_remaining;
>> -    AVPacket vpkt;
>>  } Hnm4DemuxContext;
>>
>>  static int hnm_probe(const AVProbeData *p)
>> @@ -69,55 +59,37 @@ static int hnm_read_header(AVFormatContext *s)
>>  {
>>      Hnm4DemuxContext *hnm = s->priv_data;
>>      AVIOContext *pb = s->pb;
>> +    unsigned width, height;
>>      AVStream *vst;
>>      int ret;
>>
>> -    /* default context members */
>> -    hnm->pts = 0;
>> -    av_init_packet(&hnm->vpkt);
>> -    hnm->vpkt.data = NULL;
>> -    hnm->vpkt.size = 0;
>> -
>> -    hnm->superchunk_remaining = 0;
>> -
>>      avio_skip(pb, 8);
>> -    hnm->width     = avio_rl16(pb);
>> -    hnm->height    = avio_rl16(pb);
>> -    hnm->filesize  = avio_rl32(pb);
>> +    width          = avio_rl16(pb);
>> +    height         = avio_rl16(pb);
>> +    avio_rl32(pb); // filesize
>>      hnm->frames    = avio_rl32(pb);
>> -    hnm->taboffset = avio_rl32(pb);
>> -    hnm->bits      = avio_rl16(pb);
>> -    hnm->channels  = avio_rl16(pb);
>> -    hnm->framesize = avio_rl32(pb);
>> -    avio_skip(pb, 32);
>> +    avio_skip(pb, 44);
>>
>> -    hnm->currentframe = 0;
>> -
>> -    if (hnm->width  < 256 || hnm->width  > 640 ||
>> -        hnm->height < 150 || hnm->height > 480) {
>> +    if (width  < 256 || width  > 640 ||
>> +        height < 150 || height > 480) {
>>          av_log(s, AV_LOG_ERROR,
>> -               "invalid resolution: %ux%u\n", hnm->width, hnm->height);
>> +               "invalid resolution: %ux%u\n", width, height);
>>          return AVERROR_INVALIDDATA;
>>      }
>>
>> -    // TODO: find a better way to detect HNM4A
>> -    if (hnm->width == 640)
>> -        hnm->version = 0x4a;
>> -    else
>> -        hnm->version = 0x40;
>> -
>>      if (!(vst = avformat_new_stream(s, NULL)))
>>          return AVERROR(ENOMEM);
>>
>>      vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>>      vst->codecpar->codec_id   = AV_CODEC_ID_HNM4_VIDEO;
>>      vst->codecpar->codec_tag  = 0;
>> -    vst->codecpar->width      = hnm->width;
>> -    vst->codecpar->height     = hnm->height;
>> +    vst->codecpar->width      = width;
>> +    vst->codecpar->height     = height;
>>      if ((ret = ff_alloc_extradata(vst->codecpar, 1)) < 0)
>>          return ret;
>>
>> -    vst->codecpar->extradata[0] = hnm->version;
>> +    // TODO: find a better way to detect HNM4A
>> +    vst->codecpar->extradata[0] = width == 640 ? 0x4a : 0x40;
>>
>>      vst->start_time = 0;
>>
>> @@ -186,16 +158,6 @@ static int hnm_read_packet(AVFormatContext *s, AVPacket
>> *pkt)
>>      return ret;
>>  }
>>
>> -static int hnm_read_close(AVFormatContext *s)
>> -{
>> -    Hnm4DemuxContext *hnm = s->priv_data;
>> -
>> -    if (hnm->vpkt.size > 0)
>> -        av_packet_unref(&hnm->vpkt);
>> -
>> -    return 0;
>> -}
>> -
>>  AVInputFormat ff_hnm_demuxer = {
>>      .name           = "hnm",
>>      .long_name      = NULL_IF_CONFIG_SMALL("Cryo HNM v4"),
>> @@ -203,6 +165,5 @@ AVInputFormat ff_hnm_demuxer = {
>>      .read_probe     = hnm_probe,
>>      .read_header    = hnm_read_header,
>>      .read_packet    = hnm_read_packet,
>> -    .read_close     = hnm_read_close,
>>      .flags          = AVFMT_NO_BYTE_SEEK | AVFMT_NOGENSEARCH |
>> AVFMT_NOBINSEARCH
>>  };
>> --
>> 2.20.1

Thanks, applied.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/hnm.c b/libavformat/hnm.c
index 31221553a4..f06add5cf8 100644
--- a/libavformat/hnm.c
+++ b/libavformat/hnm.c
@@ -37,19 +37,9 @@ 
 #define HNM4_CHUNK_ID_SD 17491
 
 typedef struct Hnm4DemuxContext {
-    uint8_t version;
-    uint16_t width;
-    uint16_t height;
-    uint32_t filesize;
     uint32_t frames;
-    uint32_t taboffset;
-    uint16_t bits;
-    uint16_t channels;
-    uint32_t framesize;
     uint32_t currentframe;
-    int64_t pts;
     uint32_t superchunk_remaining;
-    AVPacket vpkt;
 } Hnm4DemuxContext;
 
 static int hnm_probe(const AVProbeData *p)
@@ -69,55 +59,37 @@  static int hnm_read_header(AVFormatContext *s)
 {
     Hnm4DemuxContext *hnm = s->priv_data;
     AVIOContext *pb = s->pb;
+    unsigned width, height;
     AVStream *vst;
     int ret;
 
-    /* default context members */
-    hnm->pts = 0;
-    av_init_packet(&hnm->vpkt);
-    hnm->vpkt.data = NULL;
-    hnm->vpkt.size = 0;
-
-    hnm->superchunk_remaining = 0;
-
     avio_skip(pb, 8);
-    hnm->width     = avio_rl16(pb);
-    hnm->height    = avio_rl16(pb);
-    hnm->filesize  = avio_rl32(pb);
+    width          = avio_rl16(pb);
+    height         = avio_rl16(pb);
+    avio_rl32(pb); // filesize
     hnm->frames    = avio_rl32(pb);
-    hnm->taboffset = avio_rl32(pb);
-    hnm->bits      = avio_rl16(pb);
-    hnm->channels  = avio_rl16(pb);
-    hnm->framesize = avio_rl32(pb);
-    avio_skip(pb, 32);
+    avio_skip(pb, 44);
 
-    hnm->currentframe = 0;
-
-    if (hnm->width  < 256 || hnm->width  > 640 ||
-        hnm->height < 150 || hnm->height > 480) {
+    if (width  < 256 || width  > 640 ||
+        height < 150 || height > 480) {
         av_log(s, AV_LOG_ERROR,
-               "invalid resolution: %ux%u\n", hnm->width, hnm->height);
+               "invalid resolution: %ux%u\n", width, height);
         return AVERROR_INVALIDDATA;
     }
 
-    // TODO: find a better way to detect HNM4A
-    if (hnm->width == 640)
-        hnm->version = 0x4a;
-    else
-        hnm->version = 0x40;
-
     if (!(vst = avformat_new_stream(s, NULL)))
         return AVERROR(ENOMEM);
 
     vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
     vst->codecpar->codec_id   = AV_CODEC_ID_HNM4_VIDEO;
     vst->codecpar->codec_tag  = 0;
-    vst->codecpar->width      = hnm->width;
-    vst->codecpar->height     = hnm->height;
+    vst->codecpar->width      = width;
+    vst->codecpar->height     = height;
     if ((ret = ff_alloc_extradata(vst->codecpar, 1)) < 0)
         return ret;
 
-    vst->codecpar->extradata[0] = hnm->version;
+    // TODO: find a better way to detect HNM4A
+    vst->codecpar->extradata[0] = width == 640 ? 0x4a : 0x40;
 
     vst->start_time = 0;
 
@@ -186,16 +158,6 @@  static int hnm_read_packet(AVFormatContext *s, AVPacket *pkt)
     return ret;
 }
 
-static int hnm_read_close(AVFormatContext *s)
-{
-    Hnm4DemuxContext *hnm = s->priv_data;
-
-    if (hnm->vpkt.size > 0)
-        av_packet_unref(&hnm->vpkt);
-
-    return 0;
-}
-
 AVInputFormat ff_hnm_demuxer = {
     .name           = "hnm",
     .long_name      = NULL_IF_CONFIG_SMALL("Cryo HNM v4"),
@@ -203,6 +165,5 @@  AVInputFormat ff_hnm_demuxer = {
     .read_probe     = hnm_probe,
     .read_header    = hnm_read_header,
     .read_packet    = hnm_read_packet,
-    .read_close     = hnm_read_close,
     .flags          = AVFMT_NO_BYTE_SEEK | AVFMT_NOGENSEARCH | AVFMT_NOBINSEARCH
 };