On 7/19/17, Nicolas George <geo...@nsup.org> wrote: > Le decadi 30 messidor, an CCXXV, Rostislav Pehlivanov a ecrit : >> 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.
Except when those standards need to be applied to your work. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
