Le decadi 30 messidor, an CCXXV, Rostislav Pehlivanov a écrit : > Its a crappy format, no reason to blame anyone else but the format. > We have plenty of crappy formats which have no clear separation between > packets so demuxers have to give not-entirely-demuxed packets to the > decoder which also has to skip past junk. Its both wrong and its not, > because it isn't defined anywhere and the packets packed in the file were > never meant to go anywhere but the format they were packed in.
I mostly agree, but I would say that exactly the same applies to many image formats, especially old ones, including some amongst GIF, Sun Raster, XPM / XBM, XFace, Targa, etc. Do you disagree? > I think the image2 demuxer shouldn't handle this type of junk/useless data > filtering and would rather see a separate demuxer like the current patch > which deals with crap. I am somewhat confused by your statement, because in my mind, this is exactly the purpose and task of the image2(pipe) demuxers. The image2pipe demuxer reads the input stream, calls a parser to find in it the size of the images and returns them as individual packets. Note that the parsers are not part of the image2(pipe) demuxers, they are separate entities of their own, each specific to one image format. I do not know what the image2pipe demuxer would be good for, if not for that. Or did I miss something? Still, I do not insist that it is done with the image2(pipe) demuxer. It only seems the best solution according to what was said about the format until now. What I do insist on, is this: Look at the find_size() function in this patch: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213076.html Then look at the fits_read_header() in this patch: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213061.html They are the same. One works on an AVIOContext using read operations, the other works on a buffer using pointer arithmetic, but the logic is the same. And it is not a trivial logic, here, we are talking about dozens of lines of code. This is not good design, this is not good for maintenance: if a bug needs fixing, it will need fixing consistently in both implementations; if a feature is added, it will require the same. Good design and maintenance require a single implementation. This is especially true for an obscure format like FITS, where maintenance workforce is scarce. And this is especially true for a GSoC project, where the student is supposed to be learning good practices. I think the best and simplest way of achieving that is to have both the parser and the decoder call the same function. And I think the simplest way of implementing it is to make a parser and rely on image2pipe. Because with minimal changes to fits_read_header(), the parser would look just slightly more complex than that: static int fitsparse(AVCodecParserContext *s, AVCodecContext *avctx, const uint8_t **poutbuf, int *poutbuf_size, const uint8_t *buf, int buf_size) { fits_read_header(buf, buf_size, &size); return size; } So as it stands now, I will oppose any patch that duplicates the header-reading logic, and I think anybody should do the same, since it is everybody's responsibility to uphold the high standards of code quality in our project. Regards, -- Nicolas George
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel