On 18/01/2023 14:40, Tomas Härdin wrote:
Creating a new subthread because I just noticed something
I am a bit lost there because the line of code below is not part of this FFV1 patch. Additionally, none on my patches (FFV1 of MXF stored/sampled/displayed fix) modifies the discussed behavior (FFmpeg behavior would be same before and after this patch for MPEG-2 and AVC), so should not block any of them, and a potential fix for that should have its own patch as it would be a separate issue.
Anyway:
+ //Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced);Won't this be incorrect for files whose dimensions are multiples of 16 but not multiples of 32? Isn't each field stored separately with dimensions a multiple of 16? So while for 1080p we'll have StoredHeight = 1088 SampledHeight = 1080 and 1080i: StoredHeight = 544 SampledHeight = 540 Where 544 is a multiple of 16, for say 720p we have StoredHeight = 720 SampledHeight = 720 but for a hypothetical 720i we'd get StoredHeight = 360 SampledHeight = 360 whereas the correct values should be StoredHeight = 368 SampledHeight = 360
AFAIK, it would depend about if the stream has a picture_structure frame (16x16 applies to the frame?) of field (16x16 applies to the field?), but I really don't know enough there for having a relevant opinion.
I can just say that I don't change the behavior of FFmpeg in your use case, I found the issues when I tried a random width and height of FFV1 stream then checked with MPEG-2 Video and the sampled width was wrong for sure e.g. sampled width of 1920 for a stream having a width of 1912, with current FFmpeg code, and for your use case I am sure about nothing so I don't change the behavior with my patch, IMO if there is an issue with 720i MPEG-2 Video it should be in a separate topic and patch as it would modify the "stored_height = (st->codecpar->height+15)/16*16" current code (in my patch I just move this code), unless we are sure of what should be changed on this side and apply a fix on the way. Better to fix 1 issue and let 1 open with no change than fixing no issue because we wouldn't be sure for 1 of the 2.
Jérôme _______________________________________________ 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".
