On Thu, Jun 07, 2012 at 11:38:43AM +0200, David Girault wrote: > This new decoder will allow to decode menu stream found > in m2ts files in Bluray or AVCHD HDMV disks. > > The new decoder was developped using AVMEDIA_TYPE_OVERLAY, > inspired from AVMEDIA_TYPE_SUBTITLE, since both may send > multiple decoded pictures to add to video currently > displayed. > > version 4: > - Added some fixes for x264 demo disk that have button without > picture. > - Change palette conversion to avoid convertion in avplay.
Drop the list of changes from the log message. You can write them below the "---" line when you passs --annotate to git-send-email. > Signed-off-by: David Girault <[email protected]> > --- > libavcodec/Makefile | 1 + > libavcodec/allcodecs.c | 3 + > libavcodec/avcodec.h | 53 ++++ > libavcodec/igsmnudec.c | 743 > ++++++++++++++++++++++++++++++++++++++++++++++++ > libavcodec/utils.c | 36 +++ > libavutil/avutil.h | 1 + > 6 files changed, 837 insertions(+) > create mode 100644 libavcodec/igsmnudec.c Please read http://www.libav.org/developer.html#New-codecs-or-formats-checklist and add documentation, version and changelog updates. > --- a/libavcodec/Makefile > +++ b/libavcodec/Makefile > @@ -310,6 +310,7 @@ OBJS-$(CONFIG_PGM_ENCODER) += pnmenc.o pnm.o > OBJS-$(CONFIG_PGMYUV_ENCODER) += pnmenc.o pnm.o > OBJS-$(CONFIG_PGSSUB_DECODER) += pgssubdec.o > +OBJS-$(CONFIG_IGSMENU_DECODER) += igsmnudec.o > OBJS-$(CONFIG_PICTOR_DECODER) += pictordec.o cga_data.o > OBJS-$(CONFIG_PNG_DECODER) += png.o pngdec.o pngdsp.o alphabetical order > --- a/libavcodec/allcodecs.c > +++ b/libavcodec/allcodecs.c > @@ -375,6 +375,9 @@ void avcodec_register_all(void) > REGISTER_DECODER (SRT, srt); > REGISTER_ENCDEC (XSUB, xsub); > > + /* specific data */ > + REGISTER_DECODER (IGSMENU, igsmenu); specific? > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -3582,6 +3615,26 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, > AVSubtitle *sub, > /** > + * Decode a overlay message. an > + * Return a negative value on error, otherwise return the number of bytes > used. > + * If no overlay could be decompressed, got_sub_ptr is zero. got_overlay_ptr? > + * Otherwise, the overlay is stored in *sub. sub? > + * Note that CODEC_CAP_DR1 is not available for overlay codecs. This is for > + * simplicity, because the performance difference is expect to be negligible > + * and reusing a get_buffer written for video codecs would probably perform > badly > + * due to a potentially very different allocation pattern. > + * > + * @param avctx the codec context > + * @param[out] overlay The AVOverlay in which the decoded overlay will be > stored, must be > + freed with avoverlay_free if *got_overlay_ptr is set. > + * @param[in,out] got_overlay_ptr Zero if no overlay could be decompressed, > otherwise, it is nonzero. > + * @param[in] avpkt The input AVPacket containing the input buffer. > + */ > +int avcodec_decode_overlay2(AVCodecContext *avctx, AVOverlay *overlay, > + int *got_overlay_ptr, > + AVPacket *avpkt); Please vertically align the parameter descriptions and keep the lines below 80 characters. > --- /dev/null > +++ b/libavcodec/igsmnudec.c > @@ -0,0 +1,743 @@ > + > +#include "avcodec.h" > +#include "dsputil.h" > +#include "bytestream.h" > +#include "libavutil/colorspace.h" > +#include "libavutil/imgutils.h" nit: Move the libavutil #includes up. Below are some stylistical remarks. I haven't commented on each and every one of them. Please go through the file and fix all similar occurrences. > +#define YUVA(y,u,v,a) (((a) << 24) | ((y) << 16) | ((u) << 8) | (v)) space after comma > +typedef struct IGSButton { > + uint16_t id; > + uint16_t v; > + uint8_t f; > + uint16_t x; > + uint16_t y; > + uint16_t n[4];//0:up,1:down,2:left,3:right > + uint16_t pstart[3]; //0:normal,1:selected,2:activated spaces around // and after comma > +static av_cold int close_decoder(AVCodecContext *avctx) > +{ > + int i; > + > + IGSContext *ctx = avctx->priv_data; > + > + for (i=0;i<ctx->pictures_count;i++) { > + av_freep(&ctx->pictures[i].rle); > + } drop the {} space after ';' and around the operators > +/** > + * Decode the RLE data. > + * > + * The subtitle is stored as an Run Length Encoded image. as a > + * @param avctx contains the current codec context > + * @param ove pointer to the processed overlay data > + * @param buf pointer to the RLE data to process > + * @param buf_size size of the RLE data to process > + */ > +static int decode_rle(AVCodecContext *avctx, AVOverlay *ove, int i, > + const uint8_t *buf, unsigned int buf_size) > +{ > + const uint8_t *rle_bitmap_end; > + int pixel_count, line_count; > + > + rle_bitmap_end = buf + buf_size; > + > + ove->rects[i]->pict.data[0] = av_malloc(ove->rects[i]->w * > ove->rects[i]->h); This malloc is unchecked. > + } else if (!run) { > + /* > + * New Line. Check if correct pixels decoded, if not display > warning > + * and adjust bitmap pointer to correct new line position. > + */ Squash this onto two lines. > + if (pixel_count % ove->rects[i]->w > 0) > + av_log(avctx, AV_LOG_ERROR, "Decoded %d pixels, when line > should be %d pixels\n", > + pixel_count % ove->rects[i]->w, ove->rects[i]->w); Keep such easily-broken lines below 80 characters. > +/** > + * Parse the picture segment packet. > + * > + * The picture segment contains details on the sequence id, > + * width, height and Run Length Encoded (RLE) bitmap data. > + * > + * @param avctx contains the current codec context > + * @param buf pointer to the packet to process > + * @param buf_size size of packet to process > + * @todo TODO: Enable support for RLE data over multiple packets > + */ Return value documentation is missing. Vertically align the descriptions. > +static int parse_picture_segment(AVCodecContext *avctx, > + const uint8_t *buf, int buf_size) Indentation is off. > + /* Read the Sequence Description to determine if start of RLE data or > appended to previous RLE */ Break this long line. > + pic = &(ctx->pictures[ctx->pictures_count-1]); > + av_dlog(avctx, "Continuing bitmap @ idx %d (%p).\n", > ctx->pictures_count-1, pic); spaces around - > + pic = &(ctx->pictures[ctx->pictures_count-1]); ditto and pointless () > + av_log(avctx, AV_LOG_DEBUG, "New bitmap %04X: %dx%d @ idx %d (%p).\n", > + oid, width, height, ctx->pictures_count-1, pic); indentation > + pic->rle = av_malloc(rle_bitmap_len); > + if (!pic->rle) > + return -1; AVERROR(ENOMEM) would be a more suitable return value. > + pic->rle_buffer_size=rle_bitmap_len; spaces around = > + if (buf_size>pic->rle_buffer_size) buf_size=pic->rle_buffer_size; Don't keep statements after if on the same line. > +/** > + * Parse the palette segment packet. > + * > + * The palette segment contains details of the palette, > + * a maximum of 256 colors can be defined. > + * > + * @param avctx contains the current codec context > + * @param buf pointer to the packet to process > + * @param buf_size size of packet to process > + */ see above > +static int parse_palette_segment(AVCodecContext *avctx, > + const uint8_t *buf, int buf_size) indentation > + pal = &(ctx->palettes[ctx->palettes_count-1]); see above > + av_log(avctx, AV_LOG_DEBUG, "New palette %04X @ %p: idx=%d, size=%d, %d > colors.\n", > + oid, pal, ctx->palettes_count-1, buf_size-2, (buf_size-2)/5); spaces around operators > + buf+=1; // skip framerate (0x20 => 2 => 24fps, 0x60 => 50fps) > + buf+=2; // unknown (0x0000) > + buf+=1; // skip some flags (0x80) > + buf+=4; // skip IN_time (0xc0000163) spaces around += > + if (but->cmds_count) { > + size_t sz = 3*sizeof(uint32_t) * but->cmds_count; spaces around operators > + but->cmds = av_mallocz(sz); > + if (but->cmds) memcpy(but->cmds, buf, sz); > + buf+= sz; // skip commands unchecked malloc > +/** > + * Parse the display segment packet. > + * > + * The display segment controls the updating of the display. > + * > + * @param avctx contains the current codec context > + * @param data pointer to the data pertaining the subtitle to display > + * @param buf pointer to the packet to process > + * @param buf_size size of packet to process > + * @todo TODO: Fix start time, relies on correct PTS, currently too late > + * > + * @todo TODO: Fix end time, normally cleared by a second display > + * @todo segment, which is currently ignored as it clears > + * @todo the subtitle too early. > + */ see above I don't think you need an @todo on each line. > +static int display_end_segment(AVCodecContext *avctx, void *data, > + const uint8_t *buf, int buf_size) > +{ > + AVOverlay *ove = (AVOverlay *)data; pointless void* cast > + IGSContext *ctx = avctx->priv_data; > + IGSPage *page = NULL; > + > + int i, activated = 0; > + > + /* > + * The end display time is a timeout value and is only reached > + * if the next subtitle is later then timeout or subtitle has > + * not been cleared by a subsequent empty display command. > + */ The spacing looks weird. > + if (buf_size >= 1) { > + // this may be a fake display segment, with a navigation key embedded > + IGSButton *button=NULL; > + // handle page switch early > + if (*(const char*)buf == 'p') { Ugh, pointer type punning... > + //Todo: handle extra parameters to allow page/button selection > from HDMV command 'SetButtonPage' space after // > + if (buf_size >= 2) { > + if (buf[1]<ctx->pages_count) ctx->page = buf[1]; > + } > + else { Move the else to the previous line. > + page = &(ctx->pages[ctx->page]); > + if (buf_size >= 4) { > + page->sel_button = *(const uint16_t*)(buf+2); > + } pointless {} > + if (button != NULL) { if (button) { > + uint16_t next = button->id; > + switch(*(const char*)buf) { switch ( > + case 13: activated=1; break; > + case 'u': next = button->n[0]; break; > + case 'd': next = button->n[1]; break; > + case 'l': next = button->n[2]; break; > + case 'r': next = button->n[3]; break; Just break those lines. > + ove->num_rects = page->bogs_count; > + ove->rects = av_mallocz(sizeof(AVOverlayRect*)*ove->num_rects); This malloc is unchecked. > + rect = av_mallocz(sizeof(AVOverlayRect)); as is this one > + if (j==ctx->pictures_count) { spaces around == > + if(decode_rle(avctx, ove, i, picture->rle, > picture->rle_data_len) < 0) { if ( > + if (ret<0) { spaces around < > + av_log(avctx, AV_LOG_DEBUG, "Segment Dump:"); > + for (i = 0; i < segment_length; i++) { > + if (i%16 == 0) > + av_log(avctx, AV_LOG_DEBUG, "\n%03X: %02x", i, buf[i]); > + else > + av_log(avctx, AV_LOG_DEBUG, " %02x", buf[i]); > + } > + av_log(avctx, AV_LOG_DEBUG, "\n"); av_dlog? > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -1368,6 +1368,39 @@ void avsubtitle_free(AVSubtitle *sub) > + > +void avoverlay_free(AVOverlay *overlay) > +{ > + int i; > + > + for (i = 0; i < overlay->num_rects; i++) > + { { on the previous line > + av_freep(&overlay->rects); > + if (overlay->extra) av_freep(&overlay->extra); Break this line. Diego _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
