On Thu, Aug 03, 2017 at 12:40:04AM +0200, Tomas Härdin wrote: > + //statically assert the size of avpriv_codec2_header > + //putting it here because all codec2 things depend on codec2utils > + switch(0) { > + case 0: > + case sizeof(avpriv_codec2_header) == 7: //if false then the compiler > will complain > + break; > + }
I was pretty sure we had some kind of static assert already, based on negative array size in a struct (so it can work outside functions)... But doesn't matter due to next comments... > + *((avpriv_codec2_header*)avctx->extradata) = > avpriv_codec2_make_header(mode); I am pretty sure this is a strict aliasing violation. Even ignoring the ugliness and performance issues with returning structs and assigning them causing lots of pointless copies. Also, since all struct elements are bytes, it isn't really any more readable than just assigning to the byte array, with the names you not have in the struct in comments instead. And that is without going into all the ways someone could change that struct (e.g. in case of new features) that would completely break this kind of code due to endianness, alignment, ... Even just supporting both the current and a potential future version that changes any of the fields would be hard with this kind of code. > + if (avctx->extradata_size != sizeof(avpriv_codec2_header)) { > + av_log(avctx, AV_LOG_ERROR, "must have exactly %zu bytes of > extradata (got %i)\n", > + sizeof(avpriv_codec2_header), avctx->extradata_size); > + } I would think at least if it is less you wouldn't want to just continue? Even if extradata is required to be padded (is it?), blindly selecting 0 as mode doesn't seem very likely to be right. > + output = (int16_t *)frame->data[0]; The codec2 documentation seems pretty non-existant, so I assume you are right that this is always in native endianness. However from what I can see codec2 actually uses the "short" type, not int16_t. While it shouldn't make a difference in practice, if casting anyway I'd suggest using the type matching the API. > + if (nframes > 0) { > + *got_frame_ptr = 1; > + } Not just *got_frame_ptr = nframes > 0; ? > + int16_t *samples = (int16_t *)frame->data[0]; You are casting the const away. Did they just forget to add the const to the API? If so, can you suggest it to be added? Otherwise if it's intentional you need to make a copy. > + int ret; > + > + if ((ret = ff_alloc_packet2(avctx, avpkt, avctx->block_align, 0)) < 0) { If you merge the declaration and assignment it you would get for free not having the assignment inside the if, which I still think is just horrible style. :) > + //file starts wih 0xC0DEC2 > + if (p->buf[0] == 0xC0 && p->buf[1] == 0xDE && p->buf[2] == 0xC2) { > + return AVPROBE_SCORE_MAX; > + } As mentioned, try to find a few more bits and reduce the score. If only these 3 bytes match, I would suggest a rather low score. (I doubt there are enough useful bits in this header to allow reaching MAX score, it's a shame they didn't invest a byte or 2 more, especially considering that while 0xC0DEC2 might look clever it doesn't exactly get maximum entropy out of those 3 bytes). > + AVStream *st; > + > + if (!(st = avformat_new_stream(s, NULL)) || Can merge allocation and declaration again. > + //Read roughly 0.1 seconds worth of data. > + n = st->codecpar->bit_rate / ((int)(8/0.1) * block_align) + 1; That doesn't seem overly readable to me... // Read about 1/10th of a second worth of data st->codecpar->bit_rate / 10 / 8 / block_align + 1 Seems not really worse and doesn't have doubles and casts... Otherwise blocks_per_second = st->codecpar->bit_rate / 8 / block_align; n = blocks_per_second / 10; might be even clearer. > + if (av_new_packet(pkt, size) < 0) > + return AVERROR(ENOMEM); > + //try to read desired number of bytes, recompute n from to actual number > of bytes read > + pkt->pos= avio_tell(s->pb); > + pkt->stream_index = 0; > + ret = ffio_read_partial(s->pb, pkt->data, size); > + if (ret < 0) { > + av_packet_unref(pkt); > + return ret; > + } > + av_shrink_packet(pkt, ret); Ouch, why are you not just using av_get_packet? > + avio_write(s->pb, st->codecpar->extradata, sizeof(avpriv_codec2_header)); Error check? > + if (!(st = avformat_new_stream(s, NULL)) || > + ff_alloc_extradata(st->codecpar, sizeof(avpriv_codec2_header))) { > + return AVERROR(ENOMEM); Here and in the other read_header, this would preferably preserve the error that ff_alloc_extradata returned. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel