On Fri, Dec 27, 2024 at 12:16:04AM +0100, fishhh wrote: > > instructions on how to obtain a srv3 sample file would be helpful too. > > this is so we can actually test your patch. > > downloading the file from YouTube is probably the easiest way, > you can achieve this by the means of yt-dlp like this: > yt-dlp --skip-download --write-subs --sub-langs en --sub-format srv3 <URL> > --skip-download makes it so the video itself is not downloaded > > if you need a video with complex subtitles for testing, this > one exists: https://www.youtube.com/watch?v=gxWAM5mgH_o > > also I just realized adding `ctx->q.keep_duplicates = 1;` > somewhere in srv3_read_header significantly improves the quality > of some conversions because it keeps the queue from removing > events with the same content but different styles > this also prompted me to make EDGE_GLOW emit a smaller glow > (`{\\bord1\\blur1}`) which looks way better > you can test with those changes if you care about the result's quality
ok got working. > > usually we leave the version.h modifications out of patches posted to > > the > > mailining list, and only make those changes at git push time. this makes > > it > > easier to apply your patch in the future when those files may have > > changed. > > also for new codec we bump avcodec/version.h. > > right, makes perfect sense, the documentation at > https://ffmpeg.org/developer.html#New-codecs-or-formats-checklist > should probably be updated to not tell you to change it yourself > > > this patch sets adds a demuxer and a decoder. imho, it should be split > > into > > two patches. the decoder patch comes first, then the demuxer. > > ... > > allcodecs.c sections are sorted by name. > > okay, I'll do those things for v3. > > > > +enum SRV3PenAttrs { > > > + SRV3_PEN_ATTR_ITALIC = 1, > > > + SRV3_PEN_ATTR_BOLD = 2, > > > +}; > > > > you use 0 elsewhere in the code. > > should there be a SRV3_PEN_ATTR_NONE too? > > the difference between PenAttrs and the other enums is that > PenAttrs is a bitfield because the pen can be both italic and bold > of course. > now that I've looked into this it does seem ffmpeg uses macros > for defining flags so maybe I should just make these > #define SRV3_PEN_ATTR_ITALIC (1 << 0) > #define SRV3_PEN_ATTR_BOLD (1 << 1) > > > > +typedef struct SRV3Pen { > > > + int id; > > > + > > > + int font_size, font_style; > > > + int attrs; > > > + > > > + int edge_type, edge_color; > > > + > > > + int ruby_part; > > > > these should be enum SRV3PenAttrs, enum SRV3EdgeType, enum SRV3RubyPart, > > instead of int. this helps static analysis. HOWEVER, as part of reading > > in the values you assume they are int and this approach has merrit too. > > yeah I didn't do that specifically so I could just pass these as > int* to the parsing functions, it would be nice if we could just do > enum SRV3EdgeType : int {} > but that's a C23 thing so not really acceptable. > > > = SRV3_EDGE_NONE > > > > +#define FREE_LIST(type, list, until) \ > > > +do { \ > > > + for (void *current = list; current && current != until; current > > > = next) { \ > > > + next = ((type*)current)->next; \ > > > + av_free(current); \ > > > + } \ > > > +} while(0) > > > > syyle: the \'s should all line up > > will fix > > > > + > > > +static int srv3_probe(const AVProbeData *p) > > > +{ > > > + if (strstr(p->buf, "<timedtext format=\"3\">")) > > > + return AVPROBE_SCORE_MAX; > > > + > > > > use strstr(p->buf, "<timedtext") && strstr(p->buf, "format=\"3\""); > > > > this handle the situaton wehere an extra tag is inserted between the > > two, e.g. > > <timedtext x="y" format="3" ....> > > > > okay > > > > + > > > +static int srv3_parse_numeric_attr(SRV3Context *ctx, const char > > > *parent, xmlAttrPtr attr, int *out, int min, int max) > > > +{ > > > + return srv3_parse_numeric_value(ctx, parent, attr->name, > > > attr->children->content, 10, out, min, max) == 0; > > > +} > > > + > > > +static void srv3_parse_color_attr(SRV3Context *ctx, const char > > > *parent, xmlAttrPtr attr, int *out) > > > +{ > > > + srv3_parse_numeric_value(ctx, parent, attr->name, > > > attr->children->content + (*attr->children->content == '#'), 16, > > > out, 0, 0xFFFFFF); > > > +} > > > + > > > > for both functions, should probably return the > > srv3_parse_numeric_value() error value > > well actually the only reason srv3_parse_numeric_value returns an > error value is because attributes like "p" and "wp" have to > process the number if it is parsed successfully to find the > structure with the right id. I can make parse_color_attr return an error > but it'd be ignored in all cases. > > > srv3_parse_numeric_value() returns an error, but you are discarding it > > everywhere here. > > the error value from srv3_parse_numeric_value is discarded to allow > parsing slightly incorrect subtitles since a single event's invalid > opacity really shouldn't abort parsing and srv3_parse_numeric_value > already logs an error so its return value is only necessary in the > special cases I mentioned above. > > > i would probably convert these attributes ("sz", "fs", "et", etc.), the > > pen struct offsets, and > > min/max values into an array, and then iterate through that array > > searching for the attribute name. > > something like AVOption. > > nice idea I'll do that > > > > +static int srv3_clean_segment_text(char *text) { > > > + char *out = text; > > > + const char *start = text; > > > + > > > + while (1) { > > > + const char *end = strstr(start, ZERO_WIDTH_SPACE); > > > + size_t cnt = end ? (size_t)(end - start) : > > > (size_t)strlen(start); > > > > strlen returns size_t, should be no need to cast it. > right thanks > > > ditto for end-start; > I think pointer subtraction returns ptrdiff_t which is supposed > to be signed but it appears compilers don't complain about that > without -Wconversion (gcc doesn't even with -Wconversion which > I find amusing) since I don't see any mention of -Wconversion > in ffmpeg I can remove that cast too. > > > > + } else if (!strcmp(attr->name, "ws")) { > > > + // TODO: Handle window styles > > > > use avpriv_report_missing_feature()? > > > > I can do that but it'll trigger on many subtitle files as it's > pretty commonly used, just doesn't usually change all that much > which is why it's unimplemented. > > > > + if(textlen == 0) > > > + continue; > > > > if (textlen... > I assume you mean if(!textlen) and aren't suggesting to put > everything into an if block? actually just need to insert a space after the 'if' > > > +static const AVOption options[] = { > > > + { NULL } > > > +}; > > > + > > > +static const AVClass srv3_demuxer_class = { > > > + .class_name = "SRV3 demuxer", > > > + .option = options, > > > + .version = LIBAVUTIL_VERSION_INT, > > > +}; > > > > are you planning to use options? > > if not, options and srv3_demuxer_class can be removed. > > when I wrote this code I did have in mind a hypothetical > option that would enable using non-standard libass functionality > like BorderStyle = 3 so that's a possibility but I probably won't > implement that myself anytime soon. > can a class be added retroactively? if yes then yeah this > should probably be removed. yes class & options can be added later -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
signature.asc
Description: PGP signature
_______________________________________________ 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".
