On Sat, Mar 28, 2020 at 13:32:42 +0000, Tom Needham wrote:
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
You've inlined your patch again, and again it's corrupted by newlines.
> +unsigned char ff_dyna_callback_checksum(tBasicIdx *header)
> +{
> + int i;
> + unsigned char chksum = 0, *pHeader;
> + pHeader = (unsigned char *)header;
> + if (*pHeader == '@' && *(pHeader + 1) == '2') {
Is *header always guaranteed a size of at least two?
> + for (i = 0; i < sizeof(tBasicIdx) - 1; ++i) {
ffmpeg prefers "i++", and sizeof(variable) over sizeof(type).
> void ff_dyna_make_drv_header(tHeader *pHdr, int serno, unsigned int size, int
> pic_type)
Is this used anywhere? From its contents, it looks like a function for
a muxer, not a demuxer:
> + // set basic header
> + pHdr->Basic.Header1 = '@';
> + pHdr->Basic.Header2 = '2';
> + pHdr->Basic.QI = (pHdr->Basic.WidthBase) ? 1 : 2;
[...]
> +tHeader *ff_dyna_get_audio_header(tHeader *frame, unsigned int chksum,
> unsigned int audioserno)
> +{
> + //audio is ms adpcm. Each package size is 32(tHeader) +2048(ms adpcm).
> + tHeader *Header = frame;
> + Header->Basic.Header1 = '@';
> + Header->Basic.Header2 = '2';
> + Header->Basic.Size = 2048;
Ditto.
> + av_log(ctx, AV_LOG_DEBUG, "Parsing tBasicIdx Header \n");
You do have a log of debug messages (and a lot of empty spaces at the
end of each). They should probably not go into the final version.
> + cdata_L->Basic.NTSC_PAL = *(basicIdx_L)&0x01;
> + cdata_L->Basic.ImgMode = *(basicIdx_L) >> 1 & 0x01;
> + cdata_L->Basic.Audio = *(basicIdx_L) >> 2 & 0x01;
> + cdata_L->Basic.DST = *(basicIdx_L) >> 3 & 0x01;
> + cdata_L->Basic.Covert = *(basicIdx_L) >> 4 & 0x01;
> + cdata_L->Basic.Vloss = *(basicIdx_L) >> 5 & 0x01;
> + cdata_L->Basic.AlarmIn = *(basicIdx_L) >> 6 & 0x01;
> + cdata_L->Basic.Motion = *(basicIdx_L) >> 7 & 0x01;
> + cdata_L->Basic.ExtraEnable = *(basicIdx_L) >> 8 & 0x01;
> + cdata_L->Basic.EventEnable = *(basicIdx_L) >> 9 & 0x01;
> + cdata_L->Basic.PPS = *(basicIdx_L) >> 10 & 0x40;
> + cdata_L->Basic.Type = *(basicIdx_L) >> 16 & 0x08;
> + cdata_L->Basic.Channel = *(basicIdx_L) >> 19 & 0x20;
> + cdata_L->Basic.Chksum = *(basicIdx_L) >> 24 & 0xFF;
For readability, you could align all the '=', making it easier to see
which bits get mapped where.
(Other places as well.)
> + if (!stream_format) {
> + av_log(ctx, AV_LOG_WARNING, "File Has Unkown Stream Format:
> 0x%02X", stream_format);
> + return AVERROR_PATCHWELCOME;
Please use avpriv_report_missing_feature().
> + if (!ret) {
> + return AVERROR_PATCHWELCOME;
Also here.
> + hdr->unused_0 = (unsigned int)(pes[4] << 24 | pes[5] << 16 | pes[6] <<
> 8 | pes[7]);
> + hdr->unused_1 = (unsigned int)(pes[8] << 24 | pes[9] << 16 | pes[10]
> << 8 | pes[11]);
> + hdr->unused_2 = (unsigned int)(pes[12] << 24 | pes[13] << 16 | pes[14]
> << 8 | pes[15]);
> + hdr->unused_3 = (unsigned int)(pes[16] << 24 | pes[17] << 16 | pes[18]
> << 8 | pes[19]);
> + hdr->unused_4 = (unsigned int)(pes[20] << 24 | pes[21] << 16 | pes[22]
> << 8 | pes[23]);
There might be macros in ffmpeg for this.
> +static int dyna_read_probe(const AVProbeData *p)
> +{
> + unsigned char* bytes = av_malloc(p->buf_size);
> +
> + for (int i = 0; i < p->buf_size; i++) {
> + bytes[i] = p->buf[i];
> + }
Is this basically a memcpy?
> + if (bytes[0] == 0x40 && bytes[1] == 0x32 && bytes[2] == 0x10 &&
> bytes[3] == 0x20)
OK, but there's also a macro for this in ffmpeg.
> + tPesHeader *pes;
[...]
> + start = av_malloc(sizeof(AVIOContext));
> + memcpy(start, pb, sizeof(AVIOContext));
> +
> + pesData = av_malloc_array(PENTAMICRO_PES_HEADER_SIZE, 1);
> + pes = av_malloc(sizeof(tPesHeader));
[...]
> + ret = ff_dyna_read_file_header(ctx, pb, pesData, pes, &size, &time,
> + &priv->header, &basicIdx_H, &basicIdx_L);
> +
> + if (ret)
> + return AVERROR_INVALIDDATA;
You do realize that every malloc() has to be complemented with a
free(), even in the error case? See other demuxers: You'd have a goto
here, and a cleanup block at the end of the function, with a label.
> + size = (priv->pes_header.size_bit7to0 & 0xFF)
> + | ((priv->pes_header.size_bit10to8 & 0x04)) << 15
> + | ((priv->pes_header.size_bit14to11 & 0x08) << 11)
> + | ((priv->pes_header.size_bit21to15 & 0x7F) << 8);
Wrong indentation.
> + if (ret < 0)
> + return ret;
> +
> + return ret;
I'm not sure this makes sense. ;-)
> +static int dyna_read_close(AVFormatContext *ctx)
> +{
> + return 0;
> +}
You can probably just not define a close callback if it doesn't do
anything (but I'm not sure).
> + size = (priv->pes_header.size_bit7to0 & 0xFF)
> + | ((priv->pes_header.size_bit10to8 & 0x04)) << 15
> + | ((priv->pes_header.size_bit14to11 & 0x08) << 11)
> + | ((priv->pes_header.size_bit21to15 & 0x7F) << 8);
Wrong indentation.
> + if (avio_seek(ctx->pb, size, SEEK_SET) < 0)
> + return -1;
I'm not sure: Should you be returning an AVERROR here? (Too lazy to
check, sorry.)
> +} tBasicIdx;
ffmpeg doesn't name its structs/types this way.
Cheers,
Moritz
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
