Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
Hi, I'm bumping this patch proposal for avoiding a situation where FFmpeg skips half the visual content when 2 jpeg2000 codestreams are present in one frame. I re-reviewed this discussion and think I answered all concerns. I'm hesitant with patch v3 because I consider that touching frame_worker_thread for this feature is not so useful but I am ok with either patch v2 or v3. Thanks much, Jérôme On 20/02/2024 16:07, Jerome Martinez wrote: Attached is an updated version of the patch proposal. About the idea to keep separate fields in the output AVFrame, I note from the discussion that it is commonly accepted that up to now it is expected that the AVPacket contains what is in the MXF element and that the AVFrame contains a frame and never a field, and additionally I see in e.g. mpeg12dec.c that the decoder interleaves separate fields: mb_y <<= field_pic; if (s2->picture_structure == PICT_BOTTOM_FIELD) mb_y++; And mpegvideo_parser.c creates a AVPacket with both fields in AVPacket even if they are separated, this patch keeps the AVPacket from e.g. MXF with both fields in it and does something similar to what do other decoders with separate fields in the output AVFRame. About the detection of the 2 separated fields in 1 packet in the MXF file (I2 mode), it is doable in the example file provided in the first patch proposal to catch it by checking the essence label but other files I have don't have the specific essence label (they use the "undefined" essence label, legit) so IMO we should not rely on it and there is actually no practical advantage in catching that from the container. In practice this new patch proposal is slightly more complex, with one recursive call to jpeg2000_decode_frame() when there are 2 codestreams in 1 AVPacket, but it has a negligible performance impact (few comparisons and bool checks) when there is only one codestream in the AVPacket (the currently only supported method, remind that currently FFmpeg completely discards the 2nd codestream present in the AVPacket) and it relies on the current jpeg2000_read_main_headers() function for catching the end of the first codestream (safer than the quick find of EOC/SOC in the first version). It also changes the jpeg2000_decode_frame return value to the count of bytes parsed, it seems that it was what is expected but in practice it was not doing that, fixing the return value could be a separate patch if preferred. Jérôme On 02/02/2024 16:55, Jerome Martinez wrote: Before this patch, the FFmpeg MXF parser correctly detects content with 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the FFmpeg JPEG 2000 decoder reads the JPEG 2000 SIZ header without understanding that the indicated height is the height of 1 field only so overwrites the frame size info with e.g. 720x243, and also completely discards the second frame, which lead to the decoding of only half of the stored content as "progressive" 720x243 flagged interlaced. Example file: https://www.digitizationguidelines.gov/guidelines/MXF_sampleFiles/RDD48-sample12-gf-jpeg2000-ntsc-4.2.zip Before this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x243, lossless, SAR 9:20 DAR 4:3, 29.97 tbr, 29.97 tbn, 29.97 tbc After this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x486, lossless, SAR 9:10 DAR 4:3, 29.97 fps, 29.97 tbr, 29.97 tbn ___ 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". ___ 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". ___ 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".
Re: [FFmpeg-devel] [PATCH v3] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 01/03/2024 23:29, Tomas Härdin wrote: sön 2024-02-25 klockan 05:14 +0100 skrev Jerome Martinez: [...] I also needed to add a dedicated AVStream field for saying that the decoder is able to manage this functionality (and is needed there). What is the added value to call the decoder twice from decode.c rather than recursive call (or a master function in the decoder calling the current function twice, if preferred) inside the decoder only? We get support for all codecs that can do SEPARATE_FIELDS in MXF, rather than a half measure that only supports j2k, that we'd have to fix later anyway. I personally don't understand how, because in both cases we need a decoder aware about this feature (for the stride during decoding), anyway if you think that it will be useful in the future for another codec which would have separate fields, I warned about the issues I see in practice and it does not matter to me that it is done the way you want, my goal being that the upstream FFmpeg, rather than only my build, does not trash half of a frame (current behavior), however it is done. [...] I didn't find specifications for the essence label UL corresponding ULs aren't for specifying interlacing type (though I wouldn't be surprised if there's a mapping that misuse them for that) In practice for MXF jp2k the store method is provided by the UL (byte 15 of the essence container label), it is in the specs and all the other items (frame layout, sample rate, edit rate, aspect ratio) alone don't provide enough information (and are often buggy), I did not decide about that. About other formats and if it should not depend on the UL, I did not find any information about separate fields, difficult for me to prove that something does not exist, could you indicate another spec specifying differently separate fields? In practice and as far as I know, we currently have only jp2k with 2 completely separate codestreams in MXF, so I implemented in my patch for all existing specifications (and files) I am aware about, i.e. 1. [...] but if it appears would be only a matter of mapping the MXF signaling to the new AVStream field and supporting the feature in the decoders (even if we implement the idea of calling the decoder twice, the decoder needs to be expanded for this feature). So in other words putting it into every decoder for which there exists an MXF mapping for SEPARATE_FIELDS, rather than doing it properly? As said above, I am not convinced that calling the decoder twice from decode.c (or similar) is properly doing things, but if you think that this is properly done if it is done this way, fine for me. Patch v3 contains all the requested changes (MXF config propagation to the decoder, calling the decoder twice), is there anything else in this patch proposal preventing it to be applied? 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".
Re: [FFmpeg-devel] [PATCH v3] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 24/02/2024 13:26, Tomas Härdin wrote: [...] It should be possible to have ffmpeg set up the necessary plumbing for this. But is it how it works elsewhere in FFmpeg? Would such complex and deep modifications be accepted by others? Good question. I would propose something like the following: 1) detect the use of SEPARATE_FIELDS and set a flag in AVStream As in practice and in that case (2 jp2k codestreams per AVPacket) it is only a tip (because we can autodetect and there are many buggy files in the wild) for the jpeg2000 decoder, I was planning to add that later in a separate patch, but attached is a version with the flag. 2) allocate AVFrame for the size of the resulting *frame* So keeping what is already there. 3a) if the codec is inherently interlaced, call the decoder once 3b) if the codec is not inherently interlaced, call the decoder twice, with appropriate stride, and keep track of the number of bytes decoded so far so we know what offset to start the second decode from The place I see for that is in decode_simple_internal(). But it is a very hot place I don't like to modify, and it seems to me some extra code for 99.% (or even more 9s) of files which don't need such feature, with more risk to forget this specific feature during a future dev e.g. not obvious to change also in ff_thread_decode_frame when touching this part. I also needed to add a dedicated AVStream field for saying that the decoder is able to manage this functionality (and is needed there). What is the added value to call the decoder twice from decode.c rather than recursive call (or a master function in the decoder calling the current function twice, if preferred) inside the decoder only? As far as I understand, it would not help for other formats (only the signaling propagation in AVStream helps and it is done by another AVStream field) and I personally highly prefer that such feature is as much as possible in a single place in each decoder rather than pieces a bit everywhere, and each decoder needs to be upgraded anyway. The codecs for which 3b) applies include at least: * jpeg2000 Our use case. * ffv1 FFV1 has its own flags internally for interlaced content (interleaved method only) and I expect no work for separated fields. the MXF/FFV1 spec does not plan separated fields for FFV1, and there is no byte in the essence label for that. * rawvideo * tiff I didn't find specifications for the essence label UL corresponding and I have no file for that, as far as I understand it is highly theoretical but if it appears would be only a matter of mapping the MXF signaling to the new AVStream field and supporting the feature in the decoders (even if we implement the idea of calling the decoder twice, the decoder needs to be expanded for this feature). So IMO no dev to do there too for the moment. Jérôme From f4311b718012a92590ce6168355ec118e02052a8 Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Tue, 20 Feb 2024 16:04:11 +0100 Subject: [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket --- libavcodec/avcodec.h | 14 + libavcodec/codec_par.c | 3 ++ libavcodec/codec_par.h | 5 libavcodec/decode.c| 3 ++ libavcodec/defs.h | 7 + libavcodec/jpeg2000dec.c | 73 +++--- libavcodec/jpeg2000dec.h | 6 libavcodec/pthread_frame.c | 3 ++ libavformat/mxfdec.c | 14 + 9 files changed, 118 insertions(+), 10 deletions(-) diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 7fb44e28f4..38d63adc0f 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -2116,6 +2116,20 @@ typedef struct AVCodecContext { * an error. */ int64_t frame_num; + +/** + * Video only. The way separate fields are wrapped in the container + * - decoding: tip from the demuxer + * - encoding: not (yet) used + */ +enum AVFrameWrapping frame_wrapping; + +/** + * Video only. Indicate if running the decoder twice for a single AVFrame is supported + * - decoding: set by the decoder + * - encoding: not used + */ +intframe_wrapping_field_2_supported; } AVCodecContext; /** diff --git a/libavcodec/codec_par.c b/libavcodec/codec_par.c index abaac63841..3f26f9d4d6 100644 --- a/libavcodec/codec_par.c +++ b/libavcodec/codec_par.c @@ -51,6 +51,7 @@ static void codec_parameters_reset(AVCodecParameters *par) par->framerate = (AVRational){ 0, 1 }; par->profile = AV_PROFILE_UNKNOWN; par->level = AV_LEVEL_UNKNOWN; +par->frame_wrapping = AV_WRAPPING_UNKNOWN; } AVCodecParameters *avcodec_parameters_alloc(void) @@ -165,6 +166,7 @@ int avcodec_parameters_from_context(AVCodecParameters *par, par->sample_aspect_ratio = codec->sample_aspect_ratio; par->video_delay
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 21/02/2024 14:11, Tomas Härdin wrote: mxfdec can detect cases where there will be two separate fields in one KLV, In practice the issue is not to detect I2 case in mxfdec (it saves us only a little during probing of the first frame, but I could add such signaling in a patch after this one because it is actually independent from the management of 2 fields in the decoder if we want to support buggy files), the issue is to split per field in mxfdec. SMPTE ST 422 extracts: "Users are cautioned that the code values for SOC and EOC are not protected and can occur within the image size marker segment (SIZ), quantization marker segment (QCD), comment marker segment (COM) and other places." "Decoders can derive the bytestream offsets of each field by analysing the code stream format within the essence element as described in ISO/IEC 15444-1." Note that this MXF + jp2k spec hints that separating fields should be done in the decoder, not the demuxer. It is impossible to split per field in a codec-neutral manner due to lack of metadata for that in MXF, and doing that in mxfdec implies to duplicate jp2k header parser code in mxfdec, and to add a similar parser per supported video format in the future. and the decoder(s) can if I'm not mistaken be instructed to decode into an AVFrame with stride and offset set up for interlaced decoding. I checked the MPEG-2 Video decoder and it does not do what you say, it does what I do with this patch: - mpegvideo_parser (so the parser of raw files, equivalent to mxfdec) understands that the stream has 2 separate fields and puts them in 1 single AVPacket. It could separate them put it doesn't. - mpeg12dec (equivalent to jpeg2000dec) understands that there are 2 separate fields and applies a stride and offset internally and outputs a frame in AVFrame, not a field. It could provide fields but it doesn't. I checked only what I know well, MPEG-2 Video, maybe it is done the way you indicate elsewhere, currently I see no example about the idea that stride and offset should be provided by the demuxer, would you mind to show some code in FFmpeg doing this way? And stride and offset in signaling would mean that the target decoder must support stride and offset in signaling (including jpeg2000dec so getting a part of my current patch anyway), so no more "universal" as far as I understand. Also: Other comments say that AVPacket is expected to receive the packet as it in the container so no split of packet. Other comments say that AVFrame is expected to receive a frame and never, at least for the moment, a field. I don't see how to respect theses 2 indications, while mpegvideo_parser.c + mpeg12dec.c are conforming to theses indications because they do as in this patch. My understanding of other comments is that my patch proposal is the right way to do it, I am lost here with contradictions in the indicated path for the support of such file. e.g. https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/321540.html It should be possible to have ffmpeg set up the necessary plumbing for this. But is it how it works elsewhere in FFmpeg? Would such complex and deep modifications be accepted by others? Please confirm that it would be accepted before I go so deep in the changes of FFmpeg code, or please indicate where such tip is already stored in codec context. And it is not only plumbing, it is implementing a jp2k parser in mxfdec for splitting fields, impossible otherwise to split the fields and it is NOT for all codec at once (which is impossible in practice due to the need to have codec dependent split of fields). And in practice, what it the inconvenience to do this way, with very few modifications and in a similar manner of what is done elsewhere in FFmpeg with separate fields? Especially with patch v2 which adds nearly (a couple of comparisons between integers) no performance impact to previously supported content. As a summary, IMO this patch is conform to what is done elsewhere in FFmpeg for separated fiels e.g. in mpeg12dec and is conform to what is hinted in the specification by checking the 2nd codestream offset in the decoder rather than in the demuxer. 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".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
Attached is an updated version of the patch proposal. About the idea to keep separate fields in the output AVFrame, I note from the discussion that it is commonly accepted that up to now it is expected that the AVPacket contains what is in the MXF element and that the AVFrame contains a frame and never a field, and additionally I see in e.g. mpeg12dec.c that the decoder interleaves separate fields: mb_y <<= field_pic; if (s2->picture_structure == PICT_BOTTOM_FIELD) mb_y++; And mpegvideo_parser.c creates a AVPacket with both fields in AVPacket even if they are separated, this patch keeps the AVPacket from e.g. MXF with both fields in it and does something similar to what do other decoders with separate fields in the output AVFRame. About the detection of the 2 separated fields in 1 packet in the MXF file (I2 mode), it is doable in the example file provided in the first patch proposal to catch it by checking the essence label but other files I have don't have the specific essence label (they use the "undefined" essence label, legit) so IMO we should not rely on it and there is actually no practical advantage in catching that from the container. In practice this new patch proposal is slightly more complex, with one recursive call to jpeg2000_decode_frame() when there are 2 codestreams in 1 AVPacket, but it has a negligible performance impact (few comparisons and bool checks) when there is only one codestream in the AVPacket (the currently only supported method, remind that currently FFmpeg completely discards the 2nd codestream present in the AVPacket) and it relies on the current jpeg2000_read_main_headers() function for catching the end of the first codestream (safer than the quick find of EOC/SOC in the first version). It also changes the jpeg2000_decode_frame return value to the count of bytes parsed, it seems that it was what is expected but in practice it was not doing that, fixing the return value could be a separate patch if preferred. Jérôme On 02/02/2024 16:55, Jerome Martinez wrote: Before this patch, the FFmpeg MXF parser correctly detects content with 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the FFmpeg JPEG 2000 decoder reads the JPEG 2000 SIZ header without understanding that the indicated height is the height of 1 field only so overwrites the frame size info with e.g. 720x243, and also completely discards the second frame, which lead to the decoding of only half of the stored content as "progressive" 720x243 flagged interlaced. Example file: https://www.digitizationguidelines.gov/guidelines/MXF_sampleFiles/RDD48-sample12-gf-jpeg2000-ntsc-4.2.zip Before this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x243, lossless, SAR 9:20 DAR 4:3, 29.97 tbr, 29.97 tbn, 29.97 tbc After this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x486, lossless, SAR 9:10 DAR 4:3, 29.97 fps, 29.97 tbr, 29.97 tbn ___ 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". From 6d47d60178f50091afc3b7193b752186603efc6a Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Tue, 20 Feb 2024 16:04:11 +0100 Subject: [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket --- libavcodec/jpeg2000dec.c | 78 ++-- libavcodec/jpeg2000dec.h | 5 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c index 691cfbd891..28a3e11020 100644 --- a/libavcodec/jpeg2000dec.c +++ b/libavcodec/jpeg2000dec.c @@ -194,6 +194,8 @@ static int get_siz(Jpeg2000DecoderContext *s) int ret; int o_dimx, o_dimy; //original image dimensions. int dimx, dimy; +int previous_width = s->width; +int previous_height = s->height; if (bytestream2_get_bytes_left(>g) < 36) { av_log(s->avctx, AV_LOG_ERROR, "Insufficient space for SIZ\n"); @@ -211,7 +213,7 @@ static int get_siz(Jpeg2000DecoderContext *s) s->tile_offset_y = bytestream2_get_be32u(>g); // YT0Siz ncomponents = bytestream2_get_be16u(>g); // CSiz -if (av_image_check_size2(s->width, s->height, s->avctx->max_pixels, AV_PIX_FMT_NONE, 0, s->avctx)) { +if (av_image_check_size2(s->width, s->height << (s->has_2_fields && s->height >= 0), s->avctx->max_pixels, AV_PIX_FMT_NONE, 0, s->avctx)) { avpriv_request_sample(s->avctx, "Large Dimensions"); return AVERROR_PATCHWELCOME; } @@ -301,6 +303,20 @@ static int get_siz(Jpeg2000DecoderContext *s) return AVERROR(ENOMEM); } +/* management of frames having 2 sepa
Re: [FFmpeg-devel] [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 19/02/2024 12:08, Tomas Härdin wrote: mån 2024-02-19 klockan 00:14 +0100 skrev Tomas Härdin: tor 2024-02-15 klockan 16:02 +0100 skrev Jerome Martinez: On 05/02/2024 01:19, Tomas Härdin wrote: [...] Which entry in the table would the provided file correspond to? To me it seems none of them fit. There's two fields, meaning two j2k codestreams, in each corresponding essence element KLV packet (I think, unless CP packets get reassembled somewhere else). Entry I2 seems closest but it specifies FULL_FRAME. I1 is otherwise tempting, but there SampleRate should equal the field rate whereas the file has SampleRate = 3/1001. Other examples I have (not shareable) with 2 jp2k pictures per KLV have identification from an old version of AmberFin iCR, I have no file with the I2 correctly signaled, with my first example it isI2 (2 fields per KLV) with I1 Header Metadata Property Values **but** with I2 essence container label which has a content byte (byte 15 of the UL) of 0x04 = I2. The AmberFin iCR files have the generic essence container label with content byte of 0x01 = FU (Unspecified) so for my main use case we could activate the search of the 2nd jp2k only if I2 is explicitly signaled by the essence container label but it would prevent to catch the 2nd field when this signaling is unspecified and buggy Frame layout + sample rate + edit rate. I'm not super stoked about implementing support for broken muxers. Instead these companies should fix their code. But either way we at the very least need a reliable way to detect these kinds of files if we are to do this. There was no Software + Version information in the sample provided, which is otherwise a reliable method to deal with shitty muxers. Correction: there is Identification metadata, but it's at the end of the header metadata so I missed it. Same. Identifications Identification = {fb8e5be0-1fc5-11e9-8263-7062b8a31e5c} Identification -> Strong Reference to Identification Identification InstanceUID = {fb8e5be0-1fc5-11e9-8263-7062b8a31e5c} ThisGenerationUID = {fb8e5be0-1fc5-11e9-8264-7062b8a31e5c} CompanyName = OpenCube ProductName = MXFTk Advanced ProductUID = {3a4fe380-0d01-11e4-869f-3cd92b5c1dfc} VersionString = 2.7.0.20190123 ProductVersion = Major="2", Minor="7", Patch="0", Build="0", Release="VersionReleased" ToolkitVersion = Major="2", Minor="7", Patch="0", Build="0", Release="VersionReleased" Platform = Microsoft Windows 7 Professional Service Pack 1 (Build 7601) ModificationDate = 2019-01-24 11:51:37.884 LastModifiedDate = 2019-01-24 11:51:37.884 GenerationUID = {fb8e5be0-1fc5-11e9-8264-7062b8a31e5c} This at least (maybe) allows us to detect these broken files. But does MXFTk *always* write interlaced files like this? Beside the fact we don't know the version, I have files from other muxers having the same issue. In practice: - For the example file, no need to check identification, as said previously essence container label provides I2 tip, so I could propagate I2 tip to the jp2k decoder - But if we limit to that, some other files in the wild won't be recognized because essence container label stipulates (legally) unspecified and (out of spec but it is real) have the same issue with frame layout. If I understood correctly the other comments an AVPacket should be a MXF element so the MXF parser should not split the packet. I am preparing a v2 patch which does not include the extra probe, relying on current jp2k parser for catching the 2nd field, so in my opinion there is not value added to try to catch I2 including buggy muxers in MXF. 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".
Re: [FFmpeg-devel] [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 05/02/2024 01:19, Tomas Härdin wrote: [...] Which entry in the table would the provided file correspond to? To me it seems none of them fit. There's two fields, meaning two j2k codestreams, in each corresponding essence element KLV packet (I think, unless CP packets get reassembled somewhere else). Entry I2 seems closest but it specifies FULL_FRAME. I1 is otherwise tempting, but there SampleRate should equal the field rate whereas the file has SampleRate = 3/1001. Other examples I have (not shareable) with 2 jp2k pictures per KLV have identification from an old version of AmberFin iCR, I have no file with the I2 correctly signaled, with my first example it isI2 (2 fields per KLV) with I1 Header Metadata Property Values **but** with I2 essence container label which has a content byte (byte 15 of the UL) of 0x04 = I2. The AmberFin iCR files have the generic essence container label with content byte of 0x01 = FU (Unspecified) so for my main use case we could activate the search of the 2nd jp2k only if I2 is explicitly signaled by the essence container label but it would prevent to catch the 2nd field when this signaling is unspecified and buggy Frame layout + sample rate + edit rate. I agree that this is not what is defined in ST 422 in full, but note that frame layout and height are not required by ST 377 (only "best effort") so IMO we should not rely much on them, and at least we should handle what is in the wild, correct me if I am wrong but handling non conforming content seems an acceptable policy in FFmpeg (I think to e.g. DPX and non conforming EOLs of some scanners, their names are directly written in FFmpeg source code). Video Line Map is also best effort but without it it is not possible to know the field_order, I wonder what should be done in that case. Currently I rely on current implementation in FFmpeg for catching field_order and don't try to find the 2nd field if field_order is AV_FIELD_UNKNOWN (not important for me as all files I have have field_order related metadata). Also if I manually edit the MXF for having a conforming I2 property values, FFmpeg behaves same (still indicating half height and still silently discarding the 2nd field), so in my opinion the handling of 2 jp2k pictures per KLV is still relevant for handling correctly I2 conforming files and tolerating wrong property values may be relevant (checking essence container label only? to be discussed). On 03/02/2024 20:58, Tomas Härdin wrote: The fastest way, in a player, is probably to do it with a shader. That should be the least amount of copies and the most cache coherent. As far a I know the player is not aware that the AVFrame actually contains a field so it can not apply a shader or something else, which AVFrame field indicates that this is a a field to be interleaved with the next AVFrame before display? Currently for I1 files ffplay or VLC show double rate half height so it seems that they don't catch that AVFrame contains a field. On 03/02/2024 21:04, Tomas Härdin wrote: It should also be said that what this patch effectively does is silently convert SEPARATE_FIELDS to MIXED_FIELDS. What if I want to transcode J2K to lower bitrate but keep it SEPARATE_FIELDS? I don't get what is the expected behavior of FFmpeg: what is the meaning of "243" in "Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x243, lossless, SAR 9:20 DAR 4:3, 29.97 fps, 29.97 tbr, 29.97 tbn" My understanding is that a frame height is expected, never a field height, and there is no indication in the current output that 243 is a field height for both I1 & I2, so the "silent conversion" would be expected by the user in order to have a frame in the output and never a field, am I wrong? Also it seems that there is no way to signal that the outputted picture is a field and not a frame, and FFmpeg handles I1 (1 field per KLV) as if it ignores that this is a field and not a frame, so when a I1 file is converted to another format without an interleave filter manually added the output is an ugly flipping double rate half height content. Silently converting a field to a frame seems to me a worse behavior than silently converting SEPARATE_FIELDS to MIXED_FIELDS because the output is not what is expected by the person who created the file as well as the person watching the output of FFmpeg. What if I want to transcode J2K to lower bitrate but keep it SEPARATE_FIELDS? Interlacing the lines then encoding separately the fields? It is more a matter of default behavior (deinterlace or not) and who would need to apply a filter, my issue is that I see no way to signal in FFmpeg that "got_frame" means "got frame or field" and that AVFrame contains a field so I would prefer that the default behavior is to handle frames in AVFrame rather than fields. Is it acceptable?Additionally the MXF container indicates (for conforming files) that I2 edit rate is for a frame even
Re: [FFmpeg-devel] [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 03/02/2024 11:00, Tomas Härdin wrote: fre 2024-02-02 klockan 16:55 +0100 skrev Jerome Martinez: Before this patch, the FFmpeg MXF parser correctly detects content with 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the FFmpeg JPEG 2000 decoder reads the JPEG 2000 SIZ header without understanding that the indicated height is the height of 1 field only so overwrites the frame size info with e.g. 720x243, and also completely discards the second frame, which lead to the decoding of only half of the stored content as "progressive" 720x243 flagged interlaced. Is the decoder really the right place to do this? Surely this happens with more codecs than just j2k. Isnt it also a parser's job to do this kind of stuff? Both solutions have pro and con: - Doing it in the decoder fixes the issue for all containers and only one codec - Doing it in the demuxer fixes the issue for one container and all codecs And currently I know only one container and only one codec having this issue. My choice to implement in the decoder comes from the idea that it seems more hacky to decode 2 AVPackets (crafted from a single MXF packet), then do a RAM copy of the decoded (so huge) content for interleaving fields into 1 frame (pressure on RAM bandwidth) in the MXF demuxer + adapt frame metadata (height is overwritten by the decoder then need to overwrite again the value), doing it in the decoder seems less intrusive. If moving to the demuxer is the only acceptable solution, I will do it but I would like to be sure that there is a consensus on that by FFmpeg developers before doing it, in order to not have this extra work rejected due to a future disagreement about where it should go. The logic that scans the packet for two SOI markers shouldn't be necessary if the relevant information is carried over from the MXF demuxer. As far as I know there is nothing in the MXF file saying where is the second field in the packet, at which MXF metadata do you think? It also makes the j2k decoder harder to ||ize. You might also need the StoredF2Offset I don't change the field order detection by current FFmpeg code, I rely on FFmpeg code directly. In my opinion if MXF field order detection needs to be changed, it would be in a separate patch dedicated to that (the field order detection in MXF) and it does not impact this patch as it is independent from which container is used, using AVCodecContext field order information. 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".
[FFmpeg-devel] [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
Before this patch, the FFmpeg MXF parser correctly detects content with 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the FFmpeg JPEG 2000 decoder reads the JPEG 2000 SIZ header without understanding that the indicated height is the height of 1 field only so overwrites the frame size info with e.g. 720x243, and also completely discards the second frame, which lead to the decoding of only half of the stored content as "progressive" 720x243 flagged interlaced. Example file: https://www.digitizationguidelines.gov/guidelines/MXF_sampleFiles/RDD48-sample12-gf-jpeg2000-ntsc-4.2.zip Before this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x243, lossless, SAR 9:20 DAR 4:3, 29.97 tbr, 29.97 tbn, 29.97 tbc After this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x486, lossless, SAR 9:10 DAR 4:3, 29.97 fps, 29.97 tbr, 29.97 tbn From 5242971da7d2cf8d8713144e4a7bcc4aa06437c4 Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Thu, 1 Feb 2024 17:58:02 +0100 Subject: [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket --- libavcodec/jpeg2000dec.c | 87 +++- libavcodec/jpeg2000dec.h | 5 +++ 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c index 691cfbd891..d8bfca390e 100644 --- a/libavcodec/jpeg2000dec.c +++ b/libavcodec/jpeg2000dec.c @@ -194,6 +194,8 @@ static int get_siz(Jpeg2000DecoderContext *s) int ret; int o_dimx, o_dimy; //original image dimensions. int dimx, dimy; +int previous_width = s->width; +int previous_height = s->height; if (bytestream2_get_bytes_left(>g) < 36) { av_log(s->avctx, AV_LOG_ERROR, "Insufficient space for SIZ\n"); @@ -211,7 +213,7 @@ static int get_siz(Jpeg2000DecoderContext *s) s->tile_offset_y = bytestream2_get_be32u(>g); // YT0Siz ncomponents = bytestream2_get_be16u(>g); // CSiz -if (av_image_check_size2(s->width, s->height, s->avctx->max_pixels, AV_PIX_FMT_NONE, 0, s->avctx)) { +if (av_image_check_size2(s->width, s->height << (s->height >= 0 && s->has_2_fields), s->avctx->max_pixels, AV_PIX_FMT_NONE, 0, s->avctx)) { avpriv_request_sample(s->avctx, "Large Dimensions"); return AVERROR_PATCHWELCOME; } @@ -301,6 +303,19 @@ static int get_siz(Jpeg2000DecoderContext *s) return AVERROR(ENOMEM); } +if (s->has_2_fields) { +s->height <<= 1; +s->image_offset_y <<= 1; +s->tile_offset_y <<= 1; +if (s->is_second_field && (s->width != previous_width || s->height != previous_height)) { +avpriv_request_sample(s->avctx, "Pixel size of the 2 fields of the frame are not same"); +return AVERROR_PATCHWELCOME; +} +if (s->image_offset_y || s->tile_offset_y || (s->tile_height << 1) != s->height) { +av_log(s->avctx, AV_LOG_WARNING, "Decoding of 2 fields having titles in 1 AVPacket was not tested\n"); +} +} + /* compute image size with reduction factor */ o_dimx = ff_jpeg2000_ceildivpow2(s->width - s->image_offset_x, s->reduction_factor); @@ -2001,7 +2016,7 @@ static inline void tile_codeblocks(const Jpeg2000DecoderContext *s, Jpeg2000Tile \ y= tile->comp[compno].coord[1][0] - \ ff_jpeg2000_ceildiv(s->image_offset_y, s->cdy[compno]); \ -line = (PIXEL *)picture->data[plane] + y * (picture->linesize[plane] / sizeof(PIXEL));\ +line = (PIXEL *)picture->data[plane] + (y + (s->is_second_field ^ s->is_bottom_coded_first)) * (picture->linesize[plane] / sizeof(PIXEL));\ for (; y < h; y++) { \ PIXEL *dst; \ \ @@ -2028,7 +2043,7 @@ static inline void tile_codeblocks(const Jpeg2000DecoderContext *s, Jpeg2000Tile dst += pixelsize; \ } \ }
Re: [FFmpeg-devel] [PATCH 3/3] [RFC] avcodec/ffv1: Better rounding for slice positions
On 07/10/2023 02:14, Michael Niedermayer wrote: This fixes green lines in some odd dimensions with some slice configurations like Ticket 5548 This also changes the encoder and whats encoded, and would require an update to the specification. This change attempts to limit the change to configurations that have missing lines currently. It changes a lot the count of pixels per slice, and , e.g. with 4:2:2 and 4 slices per direction (16 slices in total), 13 pixel width: before: 3/3/3/4 for luma, 2/2/2/2 for chroma (so 1 chroma too much) after: 4/4/2/3 for luma, 2/2/1/2 for chroma Wouldn't it easier for spec and maths to keep the previous behavior for luma and consider extra chroma as to be not encoded? Something like 3/3/3/4 for luma, 2/2/2/1 for chroma Actually maybe not really a change for spec in that case, more making a part more explicit while considering the patch as a bug fix rather than a spec change. Or did I miss another issue? I'll check more a bit later. ___ 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".
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: fix check of minimal data size for unpadded content
Please consider the attached patch. Before: "Overread buffer. Invalid header?" despite that all bytes are there (the precheck is wrong, not the parsing after the precheck) After: transcoding is fine A (zeroed) sample file is available at https://trac.ffmpeg.org/ticket/10259 Jérôme On 19/10/2022 11:47, Jerome Martinez wrote: stride value is not relevant with unpadded content and the total count of pixels (width x height) must be used instead of the rounding based on width only then multiplied by height unpadded_10bit value computing is moved sooner in the code in order to be able to use it during computing of minimal content size Fix 'Overread buffer' error when the content is not lucky enough to have (enough) padding bytes at the end for not being rejected by the formula based on the stride value Signed-off-by: Jerome Martinez --- libavcodec/dpx.c | 34 ++ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 4f50608..d4699f6 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -476,14 +476,30 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p, avctx->colorspace = AVCOL_SPC_RGB; } + av_strlcpy(creator, avpkt->data + 160, 100); + creator[100] = '\0'; + av_dict_set(>metadata, "Creator", creator, 0); + + av_strlcpy(input_device, avpkt->data + 1556, 32); + input_device[32] = '\0'; + av_dict_set(>metadata, "Input Device", input_device, 0); + + // Some devices do not pad 10bit samples to whole 32bit words per row + if (!memcmp(input_device, "Scanity", 7) || + !memcmp(creator, "Lasergraphics Inc.", 18)) { + unpadded_10bit = 1; + } + // Table 3c: Runs will always break at scan line boundaries. Packing // will always break to the next 32-bit word at scan-line boundaries. // Unfortunately, the encoder produced invalid files, so attempt // to detect it + // Also handle special case with unpadded content need_align = FFALIGN(stride, 4); - if (need_align*avctx->height + (int64_t)offset > avpkt->size) { + if (need_align*avctx->height + (int64_t)offset > avpkt->size && + (!unpadded_10bit || (avctx->width * avctx->height * elements + 2) / 3 * 4 + (int64_t)offset > avpkt->size)) { // Alignment seems unappliable, try without - if (stride*avctx->height + (int64_t)offset > avpkt->size) { + if (stride*avctx->height + (int64_t)offset > avpkt->size || unpadded_10bit) { av_log(avctx, AV_LOG_ERROR, "Overread buffer. Invalid header?\n"); return AVERROR_INVALIDDATA; } else { @@ -609,20 +625,6 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p, if ((ret = ff_get_buffer(avctx, p, 0)) < 0) return ret; - av_strlcpy(creator, avpkt->data + 160, 100); - creator[100] = '\0'; - av_dict_set(>metadata, "Creator", creator, 0); - - av_strlcpy(input_device, avpkt->data + 1556, 32); - input_device[32] = '\0'; - av_dict_set(>metadata, "Input Device", input_device, 0); - - // Some devices do not pad 10bit samples to whole 32bit words per row - if (!memcmp(input_device, "Scanity", 7) || - !memcmp(creator, "Lasergraphics Inc.", 18)) { - unpadded_10bit = 1; - } - // Move pointer to offset from start of file buf = avpkt->data + offset; From 21c21373ca576f1dff05f952c17275957b9388bd Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Wed, 19 Oct 2022 11:37:34 +0200 Subject: [PATCH] avcodec/dpx: fix check of minimal data size for unpadded content stride value is not relevant with unpadded content and the total count of pixels (width x height) must be used instead of the rounding based on width only then multiplied by height unpadded_10bit value computing is moved sooner in the code in order to be able to use it during computing of minimal content size Fix 'Overread buffer' error when the content is not lucky enough to have (enough) padding bytes at the end for not being rejected by the formula based on the stride value --- libavcodec/dpx.c | 34 ++ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 4f50608461..d4699f65fc 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -476,14 +476,30 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p, avctx->colorspace = AVCOL_SPC_RGB; } +av_strlcpy(creator, avpkt->data + 160, 100); +creator[100] = '\0'; +av_dict_set(>metadata, "Creator", creator, 0); + +av_strlcpy(input_device, avpkt->data + 1556, 32); +input_device[32] = '\0'; +av_dict_set(>metadata, "Input Device", i
Re: [FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support
On 31/01/2023 15:53, Tomas Härdin wrote: sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice: On Jan 20, 2023, at 10:17 AM, Tomas Härdin wrote: ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez: 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. True, it doesn't need to hold up this patch. But some discussion is warranted I think. I might create a separate patchset for this. 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. I suspect we are lucky because 720i doesn't really exist in the real world, and 576i and 480i are both multiples of 32. IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height are "best effort" and thus shall be encoded. Their values will depend on FrameLayout which in turn depends on what you say - how exactly the interlacing is done. TL;DR: this patchset doesn't need to be held up by this. I'm just nudging on the consideration of merging this patch. I've been testing it over the last week with ffv1/mxf content and have found this demuxing support very helpful. Surely you mean muxing? Some FATE tests would be nice. Apologizes for the huge delay. Attached is an updated patch with a FATE test. Jérôme From fec4b918dd6e6a067eaeb2cd27f5e42c08dcca2c Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Tue, 14 Mar 2023 09:49:16 +0100 Subject: [PATCH] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 support --- libavformat/Makefile | 3 +- libavformat/mxfenc.c | 163 +- libavformat/rangecoder_dec.c | 1 + tests/fate/lavf-container.mak | 2 + tests/ref/lavf/mxf_ffv1 | 3 + 5 files changed, 169 insertions(+), 3 deletions(-) create mode 100644 libavformat/rangecoder_dec.c create mode 100644 tests/ref/lavf/mxf_ffv1 diff --git a/libavformat/Makefile b/libavformat/Makefile index 47fb2a..048649689b 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -712,7 +712,8 @@ SHLIBOBJS-$(CONFIG_HLS_DEMUXER) += ac3_channel_layout_tab.o SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER) += mpeg4audio_sample_rates.o SHLIBOBJS-$(CONFIG_MOV_DEMUXER) += ac3_channel_layout_tab.o SHLIBOBJS-$(CONFIG_MP3_MUXER)+= mpegaudiotabs.o -SHLIBOBJS-$(CONFIG_MXF_MUXER)+= golomb_tab.o +SHLIBOBJS-$(CONFIG_MXF_MUXER)+= golomb_tab.o \ +rangecoder_dec.o SHLIBOBJS-$(CONFIG_NUT_MUXER)+= mpegaudiotabs.o SHLIBOBJS-$(CONFIG_RTPDEC) += jpegtables.o SHLIBOBJS-$(CONFIG_RTP_MUXER)+= golomb_tab.o jpegtables.o \ diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index a29d678098..0ff566fbb4 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -51,6 +51,7 @@ #include "libavcodec/golomb.h" #include "libavcodec/h264.h"
Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height
On 10/03/2023 22:10, Marton Balint wrote: On Mon, 6 Mar 2023, Nicolas Gaullier wrote: [...] Some weeks later now and no replies, maybe time to go on ? I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate updated and that should be ok for everybody. (Ideally, it could have been an opportunity to document why we have this "DV exception", but I understand it is not very comfortable to write as there is no meaningful reason, so forget about this, this won't hold up the patch anyway) For information, there was a long thread recently on ffmpeg-user about a "bug" in dnxhd stored_height (will be fixed with your patch): https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html Will apply the patch in a couple of days unless somebody objects. If you want to change DV height (seems reasonable), please send a follow up patch with fate updates after that. Apologizes for the huge delay. Attached is an updated patch. I changed the DV part (also removed from the 16x16 macroblock thing) I added some comments about specs (summary: DNxHD is explicit, others are not but implementation I know don't do the 16x16 macroblock thing). The FATE tests for DV are not impacted because they are SD and SD width/height are multiple of 16 so I added a DV100 test. JérômeFrom c93c73164d427b2bcd29744b5a26ab82b1fe223a Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Sat, 14 Jan 2023 13:32:36 +0100 Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height Stored values are rounded to upper 16 multiple only for MPEG related formats (DV is not 16x16 macroblocks based, RDD 44 only refers to ST 377-1 without precision and no known implementation puts 1088 or 544, ST 2019-4 explicitly indicates 1080 for 1080p and 540 for 1080i) Sampled and displayed widths are codecpar ones (with DV exception) --- libavformat/mxfenc.c | 27 +-- tests/fate/lavf-container.mak | 3 ++- tests/ref/lavf/mxf_dvcpro100 | 3 +++ tests/ref/lavf/mxf_opatom | 2 +- 4 files changed, 27 insertions(+), 8 deletions(-) create mode 100644 tests/ref/lavf/mxf_dvcpro100 diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 58c551c83c..614fc5ec89 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID { MXFStreamContext *sc = st->priv_data; AVIOContext *pb = s->pb; -int stored_width = 0; -int stored_height = (st->codecpar->height+15)/16*16; +int stored_width = st->codecpar->width; +int stored_height = st->codecpar->height; +int display_width; int display_height; int f1, f2; const MXFCodecUL *color_primaries_ul; @@ -1129,12 +1130,24 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else if (st->codecpar->height == 720) stored_width = 1280; } -if (!stored_width) -stored_width = (st->codecpar->width+15)/16*16; +display_width = stored_width; +switch (st->codecpar->codec_id) { +case AV_CODEC_ID_MPEG2VIDEO: +case AV_CODEC_ID_H264: +//Based on 16x16 macroblocks +stored_width = (stored_width+15)/16*16; +stored_height = (stored_height+15)/16*16; +break; +default: +break; +} + +//Stored width mxf_write_local_tag(s, 4, 0x3203); avio_wb32(pb, stored_width); +//Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced); @@ -1154,7 +1167,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID //Sampled width mxf_write_local_tag(s, 4, 0x3205); -avio_wb32(pb, stored_width); +avio_wb32(pb, display_width); //Samples height mxf_write_local_tag(s, 4, 0x3204); @@ -1168,8 +1181,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID mxf_write_local_tag(s, 4, 0x3207); avio_wb32(pb, 0); +//Display width mxf_write_local_tag(s, 4, 0x3209); -avio_wb32(pb, stored_width); +avio_wb32(pb, display_width); if (st->codecpar->height == 608) // PAL + VBI display_height = 576; @@ -1178,6 +1192,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else display_height = st->codecpar->height; +//Display height mxf_write_local_tag(s, 4, 0x3208); avio_wb32(pb, display_height>>sc->interlaced); diff --git a/tests/fate/lavf-container.mak b/tests/fate/lavf-container.mak index 4bf7636b56..a04d31156a 100644 --- a/tests/fate/lavf-container.mak +++ b/tests/fate/lavf-container.mak @@ -8,7 +8,7 @@ FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4, MP2, MATROSKA) + FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4, PCM_ALAW, MOV)
Re: [FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support
On 18/01/2023 11:12, Tomas Härdin wrote: mån 2023-01-16 klockan 15:17 +0100 skrev Jerome Martinez: [...] I think it may be relevant to keep the exact same code for the exact same purpose. Would be no more relevant if version and micro_version can be taken from FFV1Context. Perhaps we can have the ffv1 code in lavc expose a function for this? I don't really like that we have this kind of parsing inside the muxers. This goes for MPEG-2 and H.264 as well. I don't like that too but it is beyond my skills, and as this patch does not do worse that what is already implemented, with a very small overhead (less that the MPEG-2 and H.264 parts which were accepted to be merged as is), I suggest that this is not a blocker here and this part of the code could be trashed when lavc is able to expose the codec context (if it is not yet able to do so). ___ 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".
Re: [FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support
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".
Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height
On 16/01/2023 15:00, Nicolas Gaullier wrote: Before the patch: - stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and st->codecpar->height when not MPEG formats; note that I found no other muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC) - sampled and displayed widths were stored width (should be st->codecpar->width like it is already done for height, with the DV50/100 exception) The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but xdcam does not, so current code is fine but maybe this is an opportunity to document this ? AFAIK: - DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling. - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder scaling or padding" wording with a 16x16 rounding for height, I have no file not 1920 or 3840 width - AVC in MXF: found old Omneon or old Quantel or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior there (rounding). Actually checking again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive". Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"? The height is another topic, and in my information (checked against some samples from K2/Harmonic/bmx), DV height should not be rounded : maybe this patch is an opportunity to fix this ? I don't have DV in MXF with non multiple of 16 (I thought that DV is only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of 16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: line could be definitely removed if it is fine for you. Note: please mind update fate (make fate-lavf-mxf_opatom GEN=1). Checked (only the stored height changes, from 1088 to 1080 for this DNxHD file) and fate updated for next patch. ___ 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".
Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height
On 16/01/2023 14:50, Tomas Härdin wrote: lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez: Before the patch: - stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and st->codecpar->height when not MPEG formats; note that I found no other muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC) - sampled and displayed widths were stored width (should be st->codecpar->width like it is already done for height, with the DV50/100 exception) Another option might be to omit these values for non-macroblock codecs. I planned to send a patch for removing all redundant information (sampled width/height is stored width/height by default etc), but as it is changing the current behavior of FFmpeg so can be more controversial I preferred to have it in a different patch. After this patch it would be only a bunch of "if (sampled!=stored)" additions. Would it be fine to do that separately? There is also potentially one use-value: when remuxing BMP to MXF it may be necessary to deal with BMP's 16-bit alignment. I'm not sure if this happens in the wild, and we certainly don't support muxing it. I don't get what I should change in the patch for that, AFAIK I don't touch stuff around that. Another very close reading of the spec seems appropriate. Tried to do so, as well as checking the other files I have from other muxers. When I don't get the difference between what is in specs and FFmpeg or other muxer behavior, I let current FFmpeg behavior untouched. 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".
Re: [FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support
On 16/01/2023 15:00, Tomas Härdin wrote: JPEG2000 will also need an RGBA descriptor filled out, might be good to prepare for that. this was the idea behind the way it is coded, so there is only a new mxf_write_jpeg2000_desc function to write, like the one for FFV1 i.e. static void mxf_write_jpeg2000_desc(AVFormatContext *s, AVStream *st) is_rgb = desc->flags & AV_PIX_FMT_FLAG_RGB; pos = mxf_write_cdci_common(s, st, is_rgb ? mxf_rgba_descriptor_key : mxf_cdci_descriptor_key); } to add. The ffv1 parsing code in this patch makes me nervous. Isn't the version available in metadata? I implemented a way similar to e.g. mxf_parse_mpeg2_frame by parsing a bit the frame. version and micro_version are available in FFV1Context so could be used but I don't know how to get FFV1Context from AVStream or other, need help there. +ff_build_rac_states(, 0.05 * (1LL << 32), 256 - 8); (1LL << 32) / 20 ? Could be, I don't really care, but this line is copied from ffv1dec.c, I think it may be relevant to keep the exact same code for the exact same purpose. Would be no more relevant if version and micro_version can be taken from FFV1Context. 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".
Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height
On 14/01/2023 21:04, Michael Niedermayer wrote: On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote: [...] +stored_height = (stored_height+15)/16*16; If this is supposed to match the actual macroblocks, then this would have to consider field pictures and interlacing as it differs from progressive There is no change on that side, no addition, no change about it, this code is moved, and interlace is already managed in the current code: //Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced); [...] //Display height mxf_write_local_tag(s, 4, 0x3208); avio_wb32(pb, display_height>>sc->interlaced); The patch disables the rounding for some formats, it is not intended to change the behavior for MPEG-2 Video and AVC formats as it seems fine (FFmpeg has the same behavior as other muxers, at least for MPEG-2 Video, also for interlaced content). ___ 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".
[FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height
Before the patch: - stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and st->codecpar->height when not MPEG formats; note that I found no other muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC) - sampled and displayed widths were stored width (should be st->codecpar->width like it is already done for height, with the DV50/100 exception) Could be tested with e.g. - fixed stored width (1912 instead of 1920) and height (1080 instead of 1088) not multiple of 16 : ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v jpeg2000 test_prores.mxf - fixed sampled/displayed width (1912 instead of 1920): ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v mpeg2video test_mpeg2video.mxf From cda353059886182aab2e258023c4d027c448344b Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Sat, 14 Jan 2023 13:32:36 +0100 Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height Stored values are rounded to upper 16 multiple only for MPEG related formats Sampled and displayed widths are codecpar ones (with DV exception) --- libavformat/mxfenc.c | 28 ++-- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 58c551c83c..0b7e83ba4d 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID { MXFStreamContext *sc = st->priv_data; AVIOContext *pb = s->pb; -int stored_width = 0; -int stored_height = (st->codecpar->height+15)/16*16; +int stored_width = st->codecpar->width; +int stored_height = st->codecpar->height; +int display_width; int display_height; int f1, f2; const MXFCodecUL *color_primaries_ul; @@ -1129,12 +1130,25 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else if (st->codecpar->height == 720) stored_width = 1280; } -if (!stored_width) -stored_width = (st->codecpar->width+15)/16*16; +display_width = stored_width; +switch (st->codecpar->codec_id) { +case AV_CODEC_ID_MPEG2VIDEO: +case AV_CODEC_ID_DVVIDEO: +case AV_CODEC_ID_H264: +//Based on 16x16 macroblocks +stored_width = (stored_width+15)/16*16; +stored_height = (stored_height+15)/16*16; +break; +default: +break; +} + +//Stored width mxf_write_local_tag(s, 4, 0x3203); avio_wb32(pb, stored_width); +//Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced); @@ -1154,7 +1168,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID //Sampled width mxf_write_local_tag(s, 4, 0x3205); -avio_wb32(pb, stored_width); +avio_wb32(pb, display_width); //Samples height mxf_write_local_tag(s, 4, 0x3204); @@ -1168,8 +1182,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID mxf_write_local_tag(s, 4, 0x3207); avio_wb32(pb, 0); +//Display width mxf_write_local_tag(s, 4, 0x3209); -avio_wb32(pb, stored_width); +avio_wb32(pb, display_width); if (st->codecpar->height == 608) // PAL + VBI display_height = 576; @@ -1178,6 +1193,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else display_height = st->codecpar->height; +//Display height mxf_write_local_tag(s, 4, 0x3208); avio_wb32(pb, display_height>>sc->interlaced); -- 2.13.3.windows.1 ___ 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".
[FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support
The arbitrary short element codes are the ones used by another muxer ( files available at https://www.digitizationguidelines.gov/guidelines/MXF_app_sampleFiles.html#2022 ) The support of RGBA descriptor is added, mainly by disabling in the CDCI descriptor related code the elements not in the Generic picture descriptor, and could be in a separated dedicated patch (move of Generic picture descriptor code in a dedicated function?). Tested with: ffmpeg -f lavfi -i testsrc=duration=10:size=ntsc:rate=ntsc -field_order bb -c:v ffv1 -level 0 test_ffv1_ntsc.mxf ffmpeg -f lavfi -i testsrc=duration=10:size=pal:rate=pal -field_order tt -c:v ffv1 -level 3 test_ffv1_pal.mxf ffmpeg -f lavfi -i testsrc=duration=10:size=1920x1080 -pix_fmt yuv422p10 -c:v ffv1 -level 3 test_ffv1_hd.mxf From 2e38dc0a114a1706f6d108ea9ca3e86083bfc2eb Mon Sep 17 00:00:00 2001 From: Jerome Martinez Date: Wed, 4 Jan 2023 13:56:21 +0100 Subject: [PATCH] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 support --- libavformat/Makefile | 3 +- libavformat/mxfenc.c | 163 ++- libavformat/rangecoder_dec.c | 1 + 3 files changed, 164 insertions(+), 3 deletions(-) create mode 100644 libavformat/rangecoder_dec.c diff --git a/libavformat/Makefile b/libavformat/Makefile index fa71ec12f7..3f80acbed9 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -707,7 +707,8 @@ SHLIBOBJS-$(CONFIG_HLS_DEMUXER) += ac3_channel_layout_tab.o SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER) += mpeg4audio_sample_rates.o SHLIBOBJS-$(CONFIG_MOV_DEMUXER) += ac3_channel_layout_tab.o SHLIBOBJS-$(CONFIG_MP3_MUXER)+= mpegaudiotabs.o -SHLIBOBJS-$(CONFIG_MXF_MUXER)+= golomb_tab.o +SHLIBOBJS-$(CONFIG_MXF_MUXER)+= golomb_tab.o \ +rangecoder_dec.o SHLIBOBJS-$(CONFIG_NUT_MUXER)+= mpegaudiotabs.o SHLIBOBJS-$(CONFIG_RTPDEC) += jpegtables.o SHLIBOBJS-$(CONFIG_RTP_MUXER)+= golomb_tab.o jpegtables.o \ diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 58c551c83c..206b3484aa 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -51,6 +51,7 @@ #include "libavcodec/golomb.h" #include "libavcodec/h264.h" #include "libavcodec/packet_internal.h" +#include "libavcodec/rangecoder.h" #include "libavcodec/startcode.h" #include "avformat.h" #include "avio_internal.h" @@ -99,6 +100,7 @@ typedef struct MXFStreamContext { int b_picture_count; ///< maximum number of consecutive b pictures, used in mpeg-2 descriptor int low_delay; ///< low delay, used in mpeg-2 descriptor int avc_intra; +int micro_version; ///< format micro_version, used in ffv1 descriptor } MXFStreamContext; typedef struct MXFContainerEssenceEntry { @@ -127,6 +129,7 @@ enum ULIndex { INDEX_H264, INDEX_S436M, INDEX_PRORES, +INDEX_FFV1, }; static const struct { @@ -141,6 +144,7 @@ static const struct { { AV_CODEC_ID_JPEG2000, INDEX_JPEG2000 }, { AV_CODEC_ID_H264, INDEX_H264 }, { AV_CODEC_ID_PRORES, INDEX_PRORES }, +{ AV_CODEC_ID_FFV1, INDEX_FFV1 }, { AV_CODEC_ID_NONE } }; @@ -148,6 +152,7 @@ static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st); static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st); static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st); static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st); +static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st); static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st); static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st); static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st); @@ -205,6 +210,11 @@ static const MXFContainerEssenceEntry mxf_essence_container_uls[] = { { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x17,0x00 }, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x06,0x03,0x00 }, mxf_write_cdci_desc }, +// FFV1 +{ { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23,0x01,0x00 }, + { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x1d,0x00 }, + { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x09,0x00,0x00 }, + mxf_write_ffv1_desc }, { { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 }, { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 }, { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 }, @@ -229,6 +239,15 @@ static const UID mxf_d10_container_uls[] = { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01,0x06,0
[FFmpeg-devel] [PATCH] avcodec/dpx: fix check of minimal data size for unpadded content
stride value is not relevant with unpadded content and the total count of pixels (width x height) must be used instead of the rounding based on width only then multiplied by height unpadded_10bit value computing is moved sooner in the code in order to be able to use it during computing of minimal content size Fix 'Overread buffer' error when the content is not lucky enough to have (enough) padding bytes at the end for not being rejected by the formula based on the stride value Signed-off-by: Jerome Martinez --- libavcodec/dpx.c | 34 ++ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 4f50608..d4699f6 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -476,14 +476,30 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p, avctx->colorspace = AVCOL_SPC_RGB; } + av_strlcpy(creator, avpkt->data + 160, 100); + creator[100] = '\0'; + av_dict_set(>metadata, "Creator", creator, 0); + + av_strlcpy(input_device, avpkt->data + 1556, 32); + input_device[32] = '\0'; + av_dict_set(>metadata, "Input Device", input_device, 0); + + // Some devices do not pad 10bit samples to whole 32bit words per row + if (!memcmp(input_device, "Scanity", 7) || + !memcmp(creator, "Lasergraphics Inc.", 18)) { + unpadded_10bit = 1; + } + // Table 3c: Runs will always break at scan line boundaries. Packing // will always break to the next 32-bit word at scan-line boundaries. // Unfortunately, the encoder produced invalid files, so attempt // to detect it + // Also handle special case with unpadded content need_align = FFALIGN(stride, 4); - if (need_align*avctx->height + (int64_t)offset > avpkt->size) { + if (need_align*avctx->height + (int64_t)offset > avpkt->size && + (!unpadded_10bit || (avctx->width * avctx->height * elements + 2) / 3 * 4 + (int64_t)offset > avpkt->size)) { // Alignment seems unappliable, try without - if (stride*avctx->height + (int64_t)offset > avpkt->size) { + if (stride*avctx->height + (int64_t)offset > avpkt->size || unpadded_10bit) { av_log(avctx, AV_LOG_ERROR, "Overread buffer. Invalid header?\n"); return AVERROR_INVALIDDATA; } else { @@ -609,20 +625,6 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p, if ((ret = ff_get_buffer(avctx, p, 0)) < 0) return ret; - av_strlcpy(creator, avpkt->data + 160, 100); - creator[100] = '\0'; - av_dict_set(>metadata, "Creator", creator, 0); - - av_strlcpy(input_device, avpkt->data + 1556, 32); - input_device[32] = '\0'; - av_dict_set(>metadata, "Input Device", input_device, 0); - - // Some devices do not pad 10bit samples to whole 32bit words per row - if (!memcmp(input_device, "Scanity", 7) || - !memcmp(creator, "Lasergraphics Inc.", 18)) { - unpadded_10bit = 1; - } - // Move pointer to offset from start of file buf = avpkt->data + offset; -- 2.25.1 ___ 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".
Re: [FFmpeg-devel] [PATCH] lavc/ffv1: Properly check that the 4th and 5th quant tables are zeroes
On 03/01/2020 22:59, Derek Buitenhuis wrote: On 02/01/2020 23:09, Michael Niedermayer wrote: I think if entry 128 is 0 then the whole table must be 0. If that is the case, checking the entry 128 of table 4 and 5 would be enough and caching the entry comparission is maybe not needed. Is this guaranteed somehow? It isn't mentioned in the spec. Checking better the spec about how the quant table is stored in bitstream, we may have missed in our past discussion that it is actually not stored as 256 independent values, "QuantizationTable(i, j, scale)" permits only increasing numbers from index 0 to 127 (so 128, as index 128 is the negative of index 127), so it seems that by design of how the quant table is stored it is impossible to have 0 at index 128 without having 0 at indexes 0-127 (one value of "len - 1", 127, seems to be the only way to have 0 at index 128). The need to check the 5th quant table is still valid. 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".
Re: [FFmpeg-devel] [PATCH] lavc/ffv1: Properly check that the 4th and 5th quant tables are zeroes
On 02/01/2020 16:22, Derek Buitenhuis wrote: Currently, the decoder checks the 128th value of the 4th quant table during while deriving the context on each sample, in order to speed itself up. This is due to relying on the behavior of FFmpeg's FFV1 encoder, in which if that value is zero, the entire 4th and 5th quant tables are assumed to be entirely zero. This does not match the FFV1 spec, which has no such restriction, and after some discussion, it was decided to fix FFmpeg to abide by the spec, rather than change the spec. Thanks for having taken care of this issue. [...] @@ -735,6 +737,7 @@ static int read_header(FFV1Context *f) f->chroma_h_shift, f->chroma_v_shift, f->avctx->pix_fmt); if (f->version < 2) { context_count = read_quant_tables(c, f->quant_table); +f->three_quant_table = is_three_quant_tables(f->quant_table); if (context_count < 0) { av_log(f->avctx, AV_LOG_ERROR, "read_quant_table error\n"); return AVERROR_INVALIDDATA; @@ -797,9 +800,11 @@ static int read_header(FFV1Context *f) p->quant_table_index = idx; memcpy(p->quant_table, f->quant_tables[idx], sizeof(p->quant_table)); +p->three_quant_tables = f->three_quant_tables[idx]; context_count = f->context_count[idx]; } else { memcpy(p->quant_table, f->quant_table, sizeof(p->quant_table)); +p->three_quant_tables = f->three_quant_table; } Couldn't three_quant_table be a local value instead of being stored in the context? diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c I may have missed something, but I don't get what the additional code in the encoding part does, value seems not used. And I am reluctant to change anything in the encoder in this patch as we consider that the issue is "only" a too aggressive optimization in the decoder. 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".
Re: [FFmpeg-devel] [PATCH 1/3] avcodec/rangecoder: factorize termination version code
On 19/12/2018 02:40, Michael Niedermayer wrote: Signed-off-by: Michael Niedermayer --- libavcodec/ffv1enc.c | 10 +++--- libavcodec/rangecoder.c | 4 +++- libavcodec/rangecoder.h | 2 +- libavcodec/snowenc.c | 2 +- libavcodec/sonic.c| 2 +- libavcodec/tests/rangecoder.c | 2 +- 6 files changed, 10 insertions(+), 12 deletions(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index f5eb0feb4e..796d81f7c6 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -449,7 +449,7 @@ static int write_extradata(FFV1Context *f) put_symbol(c, state, f->intra = (f->avctx->gop_size < 2), 0); } -f->avctx->extradata_size = ff_rac_terminate(c); +f->avctx->extradata_size = ff_rac_terminate(c, 0); v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, f->avctx->extradata, f->avctx->extradata_size); AV_WL32(f->avctx->extradata + f->avctx->extradata_size, v); f->avctx->extradata_size += 4; @@ -1065,9 +1065,7 @@ retry: encode_slice_header(f, fs); } if (fs->ac == AC_GOLOMB_RICE) { -if (f->version > 2) -put_rac(>c, (uint8_t[]) { 129 }, 0); -fs->ac_byte_count = f->version > 2 || (!x && !y) ? ff_rac_terminate(>c) : 0; +fs->ac_byte_count = f->version > 2 || (!x && !y) ? ff_rac_terminate(>c, f->version > 2) : 0; Moving the "129" stuff from FFV1 encoder code to rangecoder encoding part is good for factorization, but there is no more mirroring of FFV1 encoding and FFV1 decoding in that case ("129" stuff is still present in FFV1 decoder code, not rangecoder decoding part), IMO this makes the FFV1 code more difficult to understand. Isn't it possible to have the same kind of modification for the decoding part? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] ffv1enc: question about "Cannot allocate worst case packet size, the encoding could fail"
On 12/10/2018 15:30, Jan Ekström wrote: And while it seems like we're focusing on the arbitrary limit in av_malloc (which is an issue of its own), I do note that for this specific case this "worst case packed frame size" allocation heuristic might have to be modified? Or at least explained if possible, so that things can be improved in a proper way. Thank you for the feedback. I was focused on ff_alloc_packet2() which accepts 64-bit integers, and I did not check what is behind (limited to INT_MAX too). I understand now that this is a global limit, not tied to FFV1 encoder (but when this limit is removed from mem.h, there will be a review of ode which test this limit before calling ff_alloc_packet2()...). As you said in the other answer, it was unrealistic to hit such limit in 2005, but now... 2 GB by packet is still huge (the input frame is "only" 128 MB), but the multiplier used in FFV1 encoder makes the limit hit for "basic" 4K frames. 8K frames are approaching... A bit complex for me to remove this 2 GB limit for sure everywhere :(, so I try to find another solution. For historical context, this limit has been inside ffv1enc as follows, number 1 being the current version: 1. avctx->width*avctx->height*37LL*4 ("added support for AV_PIX_FMT_GBRP16" - ce2217b25eccda9f5c14022bd69792e71b417b73) 2. avctx->width*avctx->height*35LL*4 ("avcodec/ffv1enc: fix size used for ff_alloc_packet2()" - 904a2864bdafe19d18db95ca54dfb36d72957a16) 3. avctx->width*avctx->height*((8*2+1+1)*4)/8 ("ffv1enc: switch to encode2()." - 278d88689ba89c78f6c2667cf98025835567d78d) 4. ??? Limit was probably somewhere elsewhere or didn't exist before this I see it but still don't catch the reason (and I am very curious to find an input frame able to e.g. make an output frame 2x bigger than the input frame). Eager to have some background about it (Michael?). I guess there is a good reason for the "37LL*4" which is huge (note: I am afraid I did not update this limit when I implemented AV_PIX_FMT_GBRAP16, which is 4/3x bigger), so now trying to find a way to warn the user but not with 1 message per frame. If I can not (easily) remove this limitation, would limiting the message to 1 iteration for a whole stream be an acceptable patch? If so I'll send such patch. Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] ffv1enc: question about "Cannot allocate worst case packet size, the encoding could fail"
On 12/10/2018 11:15, Carl Eugen Hoyos wrote: 2018-10-11 14:01 GMT+02:00, Jerome Martinez : - why is having a number > INT_MAX an issue? modern machines are 64-bit and have 8+ GB of RAM So where is the issue? The issue is that vanilla FFmpeg shows a warning in the scenario I indicate (a 128 MB frame, far less than 2 GB), without obvious reason of this warning (the multiplier used and the underlying limitation to 2 GB, lower than the available RAM, are hard coded without comments about the reason). If above is true for you, can can simply remove the check locally, no? My goal is that **others** don't have this warning when they use a vanilla version of FFmpeg. Before sending a patch for removing the limitation I don't understand (or lowering a number I don't understand), I ask if there is a reason for it (I guess that if this code is here, there is a reason and I imagined that FFmpeg developers could kindly explain the reason), isn't it the right thing to do? Do you prefer that I send directly a patch? (I hope you agree that what you write is not generally true.) I don't catch what you want to indicate: do you mean that the only way in FFmpeg to be compatible with all machines is to set an hard coded limit to 2 GB? I don't catch the goal, as you can have machines with 1 GB so this test would be useless on them (it will not prevent other parts of the code to have to do checks about RAM allocations) as well for machines having lot of RAM (they can handle big frames). As I understand, the following test: if ((ret = ff_alloc_packet2(avctx, pkt, maxsize, 0)) < 0) return ret; permits to check that FFmpeg has enough RAM for storing the max encoded frame size, whatever is the size of the RAM (1 GB or 8 GB), so I don't understand the reason maxsize is limited to 2 GB before the call to ff_alloc_packet2. I kindly request more details about how hard coding 2 GB in the code helps, for both machines having 1 GB & machines having 8 GB. Looks like I am personally not smart enough for understanding that alone. Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] ffv1enc: question about "Cannot allocate worst case packet size, the encoding could fail"
On 11/10/2018 14:08, Paul B Mahol wrote: On 10/11/18, Jerome Martinez wrote: Hi, Testing FFmpeg FFV1 encoder on big frames (more than 4K: 4300x3956), I have a warning for each frame encoded (so a lot of warnings!): [ffv1 @ 024a6bcfe880] Cannot allocate worst case packet size, the encoding could fail Checking avcodec/ffv1enc.c, it is due to the following lines: int64_t maxsize = AV_INPUT_BUFFER_MIN_SIZE + avctx->width*avctx->height*37LL*4; [...] if (maxsize > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE - 32) { av_log(avctx, AV_LOG_WARNING, "Cannot allocate worst case packet size, the encoding could fail\n"); maxsize = INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE - 32; } The value of maxsize is 2517614784, more than INT_MAX, sure, but I don't understand: - why does FFmpeg need to allocate 148 times the size of a frame? - why is having a number > INT_MAX an issue? modern machines are 64-bit and have 8+ GB of RAM, and in practice I currently saw no encoding failure on thousands of frames. Additionally I didn't get why maxsize is reduced ("only" need of 12 times the size of the frame) in case of FFV1 version > 3 (experimental right now): if (f->version > 3) maxsize = AV_INPUT_BUFFER_MIN_SIZE + avctx->width*avctx->height*3LL*4; maxsize is used for calling ff_alloc_packet2(), which accepts 64-bit numbers. if ((ret = ff_alloc_packet2(avctx, pkt, maxsize, 0)) < 0) return ret; Is it possible to reduce this "37x4" multiplier without risk and/or remove maxsize reduction in case of sizeof(size_t) == 8 and/or show the warning only once? Issue can be reproduced with: ./ffmpeg.exe -f lavfi -i testsrc2=size=4300x3956 -t 0.040 -pix_fmt rgba64 rgba64.dpx ./ffmpeg.exe -i rgba64.dpx -c:v ffv1 rgba64.mkv You are not using latest version of FFv1. As far as I know, version 3 is the latest (stable) version of FFV1, and I use this one. version > 3 are experimental. I am not sure that advising to use an experimental version for "fixing" a warning is good. Additionally, in theory a 16kx16k content would still produce this warning with FFV1 (experimental) v4 encoding, even if there is 16 Terabytes of RAM (more than enough for having the MAX_INT = 2 GiB raw frame in RAM), so I understand that the warning would still be there (in theory) with FFV1 (experimental) v4. In latest version, encoder will not produce packets which are bigger than input ones -- uncompressed raw. And instead if that happens it will encode as raw video. Theoretically output packets for old versions of FFv1 could be several times bigger than raw input frame. In this case, the ratio (148 times the size of input) is proportional to the input frame size, and the test is on a fixed value (MAX_INT). I get now the reason to reserve more than the size of the input frame. I don't get the relationship between your explanation and these hard coded values (148 times the size of input and MAX_INT limit). Why 148x (37x4) and not 147 or 149? Why a warning and maxsize forced to MAX_INT if maxsize > MAX_INT? Is it an internal constraint of FFmpeg which would be not able to store encoded frames bigger than MAX_INT? Can any of that be changed e.g. warning only once for avoiding "spam" of such warning which hides other warnings during encoding? Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] ffv1enc: question about "Cannot allocate worst case packet size, the encoding could fail"
Hi, Testing FFmpeg FFV1 encoder on big frames (more than 4K: 4300x3956), I have a warning for each frame encoded (so a lot of warnings!): [ffv1 @ 024a6bcfe880] Cannot allocate worst case packet size, the encoding could fail Checking avcodec/ffv1enc.c, it is due to the following lines: int64_t maxsize = AV_INPUT_BUFFER_MIN_SIZE + avctx->width*avctx->height*37LL*4; [...] if (maxsize > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE - 32) { av_log(avctx, AV_LOG_WARNING, "Cannot allocate worst case packet size, the encoding could fail\n"); maxsize = INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE - 32; } The value of maxsize is 2517614784, more than INT_MAX, sure, but I don't understand: - why does FFmpeg need to allocate 148 times the size of a frame? - why is having a number > INT_MAX an issue? modern machines are 64-bit and have 8+ GB of RAM, and in practice I currently saw no encoding failure on thousands of frames. Additionally I didn't get why maxsize is reduced ("only" need of 12 times the size of the frame) in case of FFV1 version > 3 (experimental right now): if (f->version > 3) maxsize = AV_INPUT_BUFFER_MIN_SIZE + avctx->width*avctx->height*3LL*4; maxsize is used for calling ff_alloc_packet2(), which accepts 64-bit numbers. if ((ret = ff_alloc_packet2(avctx, pkt, maxsize, 0)) < 0) return ret; Is it possible to reduce this "37x4" multiplier without risk and/or remove maxsize reduction in case of sizeof(size_t) == 8 and/or show the warning only once? Issue can be reproduced with: ./ffmpeg.exe -f lavfi -i testsrc2=size=4300x3956 -t 0.040 -pix_fmt rgba64 rgba64.dpx ./ffmpeg.exe -i rgba64.dpx -c:v ffv1 rgba64.mkv Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavc/dpx: Support 10-bit packing method b (msbpad)
On 16/06/2018 17:49, Carl Eugen Hoyos wrote: Hi! Attached patch allows to decode 10-bit dpx files with packing method b, needs the 12-bit patch. Great to see the support of Method B. Please comment, Carl Eugen [...] + *lbuf = (*lbuf << 10) | (*lbuf >> shift); Padding bits are 0 in all DPX files I have seen up to now but in theory padding bits are not defined (they are set to 0 in an informative part of the spec, the one containing diagrams, which does not mean that they must be 0 as it is only an informative part; I don't see elsewhere in the spec that padding bits must be 0, I understand that value of padding bits are not defined so could be any) Example of impact on the suggested patch if padding bits are 1: Method A (shift is 22), hat is currently in FFmpeg: CCBBAA11 initial after read32 then BBAA11CC so CC with 0x3FF mask then AA11CCBB so BB with 0x3FF mask then 11CCBBAA so AA with 0x3FF mask Padding bits are never used, so they can have any value without impact on the pixels Method B (shift is 20) 11CCBBAA initial after read32 then CCBB11CC so CC with 0x3FF mask then BB11??BB so BB with 0x3FF mask then 11????11 so 11 with 0x3FF mask the last component is "corrupted" by the padding bits if they are not 0. I suggest to not expect that padding bits are 0 (to not depend on their value for filling the pixel content). For example, by using: int shift = packing == 1 ? 0 : 2; then in read10in32() add "<< shift" after read32 call: *lbuf = read32(ptr, is_big) << shift; in order to transform Method B in Method A. Similar issue with 12-bit Method B support (No 0xFFF mask so MSB bits may contain 1, which is maybe not expected in other parts of FFmpeg and may lead to weird output if padding bits are not 0, by having pixel content greater than 1^12-1) Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] avcodec/dpx: Support for RGBA 12-bit packed decoding
On 08/02/2018 11:28, Jerome Martinez wrote: Currently RGB and RGBA 12-bit are supported by DPX decoder only if component values are padded (packing "Filled to 32-bit words, method A"). This patch adds decoding of RGB and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). As I have no file with line boundaries not aligned on 32-bit, I can not have good tests about the stride computing (so code about non aligned boundaries is theory) so I preferred to limit risks by decoding only if line boundaries are aligned on 32-bit words: - 8 pixels for RGB (8 pixels x 3 components x 12 bits = 288 bits = 9 x 32-bit words) - 2 pixels for RGBA (2 pixels x 4 components x 12 bits = 3 x 32-bit words) I think Little Endian parsing is fine thanks to the generic code about Big vs Little endian but I have no Little Endian test file so I also limited the decoding to Big Endian files. Would be happy to check with cases I was not able to check if someone provides files. Since the email, RGB 12-bit packed decoding was pushed. Attached is the patch for RGBA 12-bit packed decoding, rebased. Test file: https://trac.ffmpeg.org/attachment/ticket/5639/12bit_a.dpx From b22c71ed0d81bce8a0a0116052970f8a9bcc9008 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= Date: Fri, 1 Jun 2018 10:09:01 +0200 Subject: [PATCH] avcodec/dpx: Support for RGBA 12-bit packed decoding Limited to widths multiple of 2 due to lack of test files for such corner case This partially fixes ticket #5639 --- libavcodec/dpx.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 026fb10e90..fb388b6e52 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -237,6 +237,9 @@ static int decode_frame(AVCodecContext *avctx, if (descriptor == 50 && endian && (avctx->width%8) == 0) { // Little endian and widths not a multiple of 8 need tests tested = 1; } +if (descriptor == 51 && endian && (avctx->width%2) == 0) { // Little endian and widths not a multiple of 2 need tests +tested = 1; +} if (!tested) { av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); return -1; -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB 12-bit packed decoding
On 11/04/2018 00:16, Lou Logan wrote: On Tue, Apr 10, 2018, at 2:05 PM, Michael Niedermayer wrote: what do you mean by "Git and me are not good friends" ? If git hates you and sometimes does things that you didnt intend at all then that would be a problem with direct pushes as theres no way to undo. But maybe i misunderstand. Yes, that's right :), I think you also remarked that I am an expert of careless mistakes :(. I am still at the bottom of the Git learning curve, being more used to the GitHub PR things (which IMO avoids well to do a mess with upstream repo). Additionally, I think that it does not worth it to take risks (including the risk of compromised machine) about giving me write rights with the only few patches I send. It would be another story if I become more active on FFmpeg code in the future. Also to get git write access, post a patch that adds yourself to the MAINTAINERs file. When noone objects then ill add your key and apply the MAINTAINER patch. Also, if you're uncomfortable pushing to the repository that's fine too. It is not a problem, but you may have to prod and remind us, perhaps a few times (or more), to push stuff. I kindly request a maintainer to push one (the latest one?) of the DPX patches in this thread (i.e. right I am uncomfortable pushing to FFmpeg repo directly, I would do it only if it is the only method for having my patches pushed upstream after review due to no current maintainer willing to push them), as well as reviewing then push (if they are fine) the ones related to FFV1 [1] Jérôme [1] https://ffmpeg.org/pipermail/ffmpeg-devel/2018-March/226192.html ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB 12-bit packed decoding
On 10/04/2018 12:34, Carl Eugen Hoyos wrote: 2018-04-10 12:28 GMT+02:00, Kieran O Leary: I just tested this patch non packed to 16-bit gbrp12le DPX from DaVinci Resolve. Testing is good, apart I thought the patch was "technically" OK, as I answered to all change requests and there was no additional feedback IIRC. from more brackets Not sure I understand, as the only "missing" brackets I see are for the 1 line code after a "if", and I see that 1 line code has no brackets in other parts of the file. Anyway, I added more brackets, except for "if (*n_datum) (*n_datum)--;" as I copied/pasted it from another part of the file. Did I miss something else? (and less comments) I thought it would be better for someone willing to add alpha support in the future, as the alpha support was tested and "just" rejected for the moment. Anyway, I removed the commented code. Modified patch attached. Note that I personally prefer to use the previous patch (or this patch without the additional brackets). it would be better if Jerome sends his public keys to Michael and pushes the patch. If it is the only solution for having the patch pushed, I'll do that, even if I am not convinced that I deserve for the moment write rights on FFmpeg repository (especially because Git and me are not good friends :) ). Jérôme From 02286a07f920b8d15bf5f031a21fc9080fc4fa32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= Date: Tue, 10 Apr 2018 18:20:23 +0200 Subject: [PATCH] avcodec/dpx: Support for RGB 12-bit packed decoding Limited to widths multiple of 8 (RGB) due to lack of test files for such corner case This partially fixes ticket #5639 --- libavcodec/dpx.c | 67 +--- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 1aa2cbd1c8..026fb10e90 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -65,6 +65,38 @@ static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf, return *lbuf & 0x3FF; } +static uint16_t read12in32(const uint8_t **ptr, uint32_t * lbuf, + int * n_datum, int is_big) +{ +if (*n_datum) +(*n_datum)--; +else { +*lbuf = read32(ptr, is_big); +*n_datum = 7; +} + +switch (*n_datum){ +case 7: return *lbuf & 0xFFF; +case 6: return (*lbuf >> 12) & 0xFFF; +case 5: { +uint32_t c = *lbuf >> 24; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 8; +return c & 0xFFF; +} +case 4: return (*lbuf >> 4) & 0xFFF; +case 3: return (*lbuf >> 16) & 0xFFF; +case 2: { +uint32_t c = *lbuf >> 28; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 4; +return c & 0xFFF; +} +case 1: return (*lbuf >> 8) & 0xFFF; +default: return *lbuf >> 20; +} +} + static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, @@ -201,10 +233,27 @@ static int decode_frame(AVCodecContext *avctx, break; case 12: if (!packing) { -av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); -return -1; +int tested = 0; +if (descriptor == 50 && endian && (avctx->width%8) == 0) { // Little endian and widths not a multiple of 8 need tests +tested = 1; +} +if (!tested) { +av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); +return -1; +} +} +stride = avctx->width * elements; +if (packing) { +stride *= 2; +} else { +stride *= 3; +if (stride % 8) { +stride /= 8; +stride++; +stride *= 8; +} +stride /= 2; } -stride = 2 * avctx->width * elements; break; case 16: stride = 2 * avctx->width * elements; @@ -349,6 +398,7 @@ static int decode_frame(AVCodecContext *avctx, (uint16_t*)ptr[2], (uint16_t*)ptr[3]}; for (y = 0; y < avctx->width; y++) { +if (packing) { if (elements >= 3) *dst[2]++ = read16(, endian) >> 4; *dst[0] = read16(, endian) >> 4; @@ -357,6 +407,17 @@ static int decode_frame(AVCodecContext *avctx, *dst[1]++ = read16(, endian) >> 4; if (elements == 4) *dst[3]++ = read16(, endian) >> 4; +} else { +*dst[2]++ = read12in32(, , + _datum, endian); +*dst[0]++ = read12in32(, , + _datum, endian); +
Re: [FFmpeg-devel] [PATCH 6/7] avcodec/ffv1: support of more pix_fmt
On 09/03/2018 18:30, Paul B Mahol wrote: On 3/7/18, Jerome Martinez <jer...@mediaarea.net> wrote: With some sources having specific pix_fmt (9/10/12/14 bit), the source is padded to 16-bit when the pix_fmt is not supported natively by the FFV1 encoder. Nothing is lost ("cutting" to the source bitdepth permits to retrieve the exact source), but the source bitdepth is not indicated so it is not possible to retrieve the exact source without having the source bitdepth by another channel. It also makes FATE tests (framemd5 comparison) a bit more complicated (bitdepth reduction). This patch adds native support of the pix_fmt being padded, so there is no more padding and the FFV1 bitdepth is same as source bitdepth. Trailing whitespaces are not allowed in FFmpeg codebase. Was not intended. Updated patch attached. From 6e9a50ba90c3264c26b4e775e0618ee1702812a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Wed, 7 Mar 2018 11:19:03 +0100 Subject: [PATCH] avcodec/ffv1: support of more pix_fmt Without direct support of such pix_fmt, content is padded to 16-bit and it is not possible to know that the source file was with a smaller bit depth so framemd5 is different --- libavcodec/ffv1dec.c | 14 +- libavcodec/ffv1enc.c | 13 - 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 3d2ee2569f..b4a183c5b7 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -591,7 +591,10 @@ static int read_header(FFV1Context *f) if (!f->transparency && !f->chroma_planes) { if (f->avctx->bits_per_raw_sample <= 8) f->avctx->pix_fmt = AV_PIX_FMT_GRAY8; -else if (f->avctx->bits_per_raw_sample == 10) { +else if (f->avctx->bits_per_raw_sample == 9) { +f->packed_at_lsb = 1; +f->avctx->pix_fmt = AV_PIX_FMT_GRAY9; +} else if (f->avctx->bits_per_raw_sample == 10) { f->packed_at_lsb = 1; f->avctx->pix_fmt = AV_PIX_FMT_GRAY10; } else if (f->avctx->bits_per_raw_sample == 12) { @@ -642,6 +645,7 @@ static int read_header(FFV1Context *f) f->packed_at_lsb = 1; switch(16 * f->chroma_h_shift + f->chroma_v_shift) { case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUV444P10; break; +case 0x01: f->avctx->pix_fmt = AV_PIX_FMT_YUV440P10; break; case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUV422P10; break; case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUV420P10; break; } @@ -656,9 +660,17 @@ static int read_header(FFV1Context *f) f->packed_at_lsb = 1; switch(16 * f->chroma_h_shift + f->chroma_v_shift) { case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUV444P12; break; +case 0x01: f->avctx->pix_fmt = AV_PIX_FMT_YUV440P12; break; case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUV422P12; break; case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUV420P12; break; } +} else if (f->avctx->bits_per_raw_sample == 14 && !f->transparency) { +f->packed_at_lsb = 1; +switch(16 * f->chroma_h_shift + f->chroma_v_shift) { +case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUV444P14; break; +case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUV422P14; break; +case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUV420P14; break; +} } else if (f->avctx->bits_per_raw_sample == 16 && !f->transparency){ f->packed_at_lsb = 1; switch(16 * f->chroma_h_shift + f->chroma_v_shift) { diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index d71d952c6d..60698827b6 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -558,6 +558,7 @@ FF_ENABLE_DEPRECATION_WARNINGS s->plane_count = 3; switch(avctx->pix_fmt) { +case AV_PIX_FMT_GRAY9: case AV_PIX_FMT_YUV444P9: case AV_PIX_FMT_YUV422P9: case AV_PIX_FMT_YUV420P9: @@ -568,6 +569,7 @@ FF_ENABLE_DEPRECATION_WARNINGS s->bits_per_raw_sample = 9; case AV_PIX_FMT_GRAY10: case AV_PIX_FMT_YUV444P10: +case AV_PIX_FMT_YUV440P10: case AV_PIX_FMT_YUV420P10: case AV_PIX_FMT_YUV422P10: case AV_PIX_FMT_YUVA444P10: @@ -577,11 +579,17 @@ FF_ENABLE_DEPRECATION_WARNINGS s->bits_per_raw_sample = 10; case AV_PIX_FMT_GRAY12: case AV_PIX_FMT_YUV444P12: +case AV_PIX_FMT_YUV440P12: case AV_PIX_FMT_YUV420P12: case AV_PIX_FMT_YUV422P12: -s->packed_at_lsb = 1; if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample) s->b
Re: [FFmpeg-devel] [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder
On 09/03/2018 03:15, Michael Niedermayer wrote: On Thu, Mar 08, 2018 at 03:12:26AM +0100, Jerome Martinez wrote: checking range coder part, I see that currently there is actually a slight difference with the other AV_LOG_INFO, I don't indicate the message when level is not indicated, as I didn't see the value added of such information when level is not indicated on the command line when I implemented the test. With patch proposal: ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 a.mkv [ffv1 @ 0232D35B9240] bits_per_raw_sample > 8, forcing range coder (no change) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -coder 0 a.mkv [ffv1 @ 014B9B1771C0] bits_per_raw_sample > 8, forcing range coder (no change) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 a.mkv [ffv1 @ 01BC2B881500] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01BC2B881500] bits_per_raw_sample > 8, forcing range coder ("version 1" line added because user specified version 0, IMO encoder should inform user that this is not respected) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv [ffv1 @ 01EAC7685180] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01EAC7685180] bits_per_raw_sample > 8, forcing range coder ("version 1" line added because user specified version 0, IMO encoder should inform user that this is not respected) I have the suspicion there are more corner cases that behave odd, but ive no time ATM to test, ill do that tomorrow unless i forget I'll add some tests about respecting or displaying the change compared to the command line in FATE. Just for being clear: do you want info message also when level/version is *not* indicated on the command line? some sort of debug level message indicating the version may be usefull if theres nothing showing it currently For debug log, I suggest to provide all impacting config items, i.e. version, slice count, coder type... at higher level i think it should only be shown if its worth it to the user like for example if it overrides what was speified or somehow has compatibility issues So my patch is fine, and there is the need to change the current behavior for the other log line, I'll do another patch. But for the moment I prefer to have the already FFV1 related pending items (7 FFmpeg patches and 5 FFV1 spec patches) handled for better seeing the direction to go before adding new items to the list. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder
On 08/03/2018 01:33, Michael Niedermayer wrote: On Wed, Mar 07, 2018 at 04:49:24PM +0100, Jerome Martinez wrote: There is a message when coder type is forced to a value not chosen by user, but no message when version is forced to a value not chosen by user. This patch adds such message for more coherency in the messages, and the user is informed that the command is not fully respected. ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv Before: [ffv1 @ 02492CD69B40] bits_per_raw_sample > 8, forcing range coder After: [ffv1 @ 01A6E404A780] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01A6E404A780] bits_per_raw_sample > 8, forcing range coder ffv1enc.c |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) cb1df919e21fe4d388df7de9349c5c2c46777862 0002-avcodec-ffv1enc-add-information-message-when-version.patch From 49db6049fa50976683fc651cf180ab8c7428225e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Wed, 7 Mar 2018 10:37:46 +0100 Subject: [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder --- libavcodec/ffv1enc.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index d71d952c6d..ac8b715b74 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -509,7 +509,7 @@ static av_cold int encode_init(AVCodecContext *avctx) { FFV1Context *s = avctx->priv_data; const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); -int i, j, k, m, ret; +int i, j, k, m, ret, oldversion; if ((ret = ff_ffv1_common_init(avctx)) < 0) return ret; @@ -534,6 +534,7 @@ static av_cold int encode_init(AVCodecContext *avctx) } s->version = avctx->level; } +oldversion = s->version; if (s->ec < 0) { s->ec = (s->version >= 3); @@ -679,6 +680,11 @@ FF_ENABLE_DEPRECATION_WARNINGS av_assert0(s->bits_per_raw_sample >= 8); if (s->bits_per_raw_sample > 8) { +if (oldversion >= 0 && oldversion != s->version) { +av_log(avctx, AV_LOG_INFO, +"bits_per_raw_sample > 8, forcing version 1\n"); +oldversion = s->version; +} I dont think this works consistently The code does not seem to be limited to the case where the user has specifified a version for example unless i miss something checking range coder part, I see that currently there is actually a slight difference with the other AV_LOG_INFO, I don't indicate the message when level is not indicated, as I didn't see the value added of such information when level is not indicated on the command line when I implemented the test. With patch proposal: ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 a.mkv [ffv1 @ 0232D35B9240] bits_per_raw_sample > 8, forcing range coder (no change) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -coder 0 a.mkv [ffv1 @ 014B9B1771C0] bits_per_raw_sample > 8, forcing range coder (no change) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 a.mkv [ffv1 @ 01BC2B881500] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01BC2B881500] bits_per_raw_sample > 8, forcing range coder ("version 1" line added because user specified version 0, IMO encoder should inform user that this is not respected) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv [ffv1 @ 01EAC7685180] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01EAC7685180] bits_per_raw_sample > 8, forcing range coder ("version 1" line added because user specified version 0, IMO encoder should inform user that this is not respected) Just for being clear: do you want info message also when level/version is *not* indicated on the command line? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/7] avcodec/ffv1dec: add missing error messages when a frame is invalid
On 08/03/2018 01:17, Michael Niedermayer wrote: In the cases where the error is about a scalar value, that value should be printed in the error message (unless it was alread printed elsewhere) Patch modified, showing the scalar value. From 13db9fc4da1d0e531e516bd87d084233e231f0e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Thu, 8 Mar 2018 02:40:21 +0100 Subject: [PATCH] avcodec/ffv1dec: add missing error messages when a frame is invalid --- libavcodec/ffv1dec.c | 39 +++ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 3d2ee2569f..06509dae60 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -182,11 +182,22 @@ static int decode_slice_header(FFV1Context *f, FFV1Context *fs) fs->slice_y /= f->num_v_slices; fs->slice_width = fs->slice_width /f->num_h_slices - fs->slice_x; fs->slice_height = fs->slice_height/f->num_v_slices - fs->slice_y; -if ((unsigned)fs->slice_width > f->width || (unsigned)fs->slice_height > f->height) +if ((unsigned)fs->slice_width > f->width) { +av_log(f->avctx, AV_LOG_ERROR, "slice_width %d out of range\n", (unsigned)fs->slice_width); return -1; -if ((unsigned)fs->slice_x + (uint64_t)fs->slice_width > f->width - || (unsigned)fs->slice_y + (uint64_t)fs->slice_height > f->height) +} +if ((unsigned)fs->slice_height > f->height) { +av_log(f->avctx, AV_LOG_ERROR, "slice_height %d out of range\n", (unsigned)fs->slice_height); +return -1; +} +if ((unsigned)fs->slice_x + (uint64_t)fs->slice_width > f->width) { +av_log(f->avctx, AV_LOG_ERROR, "slice_x+slice_width %lld out of range\n", (unsigned)fs->slice_x + (uint64_t)fs->slice_width, (unsigned)fs->slice_y); return -1; +} +if ((unsigned)fs->slice_y + (uint64_t)fs->slice_height > f->height) { +av_log(f->avctx, AV_LOG_ERROR, "slice_y+slice_height %lld out of range\n", (unsigned)fs->slice_y + (uint64_t)fs->slice_height); +return -1; +} for (i = 0; i < f->plane_count; i++) { PlaneContext * const p = >plane[i]; @@ -432,8 +443,10 @@ static int read_extra_header(FFV1Context *f) if (f->version > 2) { c->bytestream_end -= 4; f->micro_version = get_symbol(c, state, 0); -if (f->micro_version < 0) +if (f->micro_version < 0) { +av_log(f->avctx, AV_LOG_ERROR, "Invalid micro_version %i in global header\n", f->micro_version); return AVERROR_INVALIDDATA; +} } f->ac = get_symbol(c, state, 0); @@ -758,12 +771,22 @@ static int read_header(FFV1Context *f) fs->slice_y /= f->num_v_slices; fs->slice_width = fs->slice_width / f->num_h_slices - fs->slice_x; fs->slice_height = fs->slice_height / f->num_v_slices - fs->slice_y; -if ((unsigned)fs->slice_width > f->width || -(unsigned)fs->slice_height > f->height) +if ((unsigned)fs->slice_width > f->width) { +av_log(f->avctx, AV_LOG_ERROR, "slice_width %d out of range\n", (unsigned)fs->slice_width); return AVERROR_INVALIDDATA; -if ( (unsigned)fs->slice_x + (uint64_t)fs->slice_width > f->width -|| (unsigned)fs->slice_y + (uint64_t)fs->slice_height > f->height) +} +if ((unsigned)fs->slice_height > f->height) { +av_log(f->avctx, AV_LOG_ERROR, "slice_height %d out of range\n", (unsigned)fs->slice_height); +return AVERROR_INVALIDDATA; +} +if ((unsigned)fs->slice_x + (uint64_t)fs->slice_width > f->width) { +av_log(f->avctx, AV_LOG_ERROR, "slice_x+slice_width %lld out of range\n", (unsigned)fs->slice_x + (uint64_t)fs->slice_width, (unsigned)fs->slice_y); return AVERROR_INVALIDDATA; +} +if ((unsigned)fs->slice_y + (uint64_t)fs->slice_height > f->height) { +av_log(f->avctx, AV_LOG_ERROR, "slice_y+slice_height %lld out of range\n", (unsigned)fs->slice_y + (uint64_t)fs->slice_height); +return AVERROR_INVALIDDATA; +} } for (i = 0; i < f->plane_count; i++) { -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 7/7] avcodec/ffv1enc: remove warning about transparency
The message about the need of a recent FFmpeg version when encoding alpha plane is 5+ year old, not really relevant anymore. This patch removes the message. From 8e3bbad708b5a3a24920133c5bef0b7399375393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 13:26:36 +0100 Subject: [PATCH 7/7] avcodec/ffv1enc: remove warning about transparency --- libavcodec/ffv1enc.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index f0f9eaba79..c6e11a3f55 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -699,9 +699,6 @@ FF_ENABLE_DEPRECATION_WARNINGS s->ac = AC_RANGE_CUSTOM_TAB; } } -if (s->transparency) { -av_log(avctx, AV_LOG_WARNING, "Storing alpha plane, this will require a recent FFV1 decoder to playback!\n"); -} #if FF_API_PRIVATE_OPT FF_DISABLE_DEPRECATION_WARNINGS if (avctx->context_model) -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 7/7] avcodec/ffv1enc: remove warning about transparency
The message about the need of a recent FFmpeg version when encoding alpha plane is 5+ year old, not really relevant anymore. This patch removes the message. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 6/7] avcodec/ffv1: support of more pix_fmt
With some sources having specific pix_fmt (9/10/12/14 bit), the source is padded to 16-bit when the pix_fmt is not supported natively by the FFV1 encoder. Nothing is lost ("cutting" to the source bitdepth permits to retrieve the exact source), but the source bitdepth is not indicated so it is not possible to retrieve the exact source without having the source bitdepth by another channel. It also makes FATE tests (framemd5 comparison) a bit more complicated (bitdepth reduction). This patch adds native support of the pix_fmt being padded, so there is no more padding and the FFV1 bitdepth is same as source bitdepth. From 8efae7dd23ac30a84161e3d5a46b38350703a7a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 11:19:03 +0100 Subject: [PATCH 6/7] avcodec/ffv1: support of more pix_fmt Without direct support of such pix_fmt, content is padded to 16-bit and it is not possible to know that the source file was with a smaller bit depth so framemd5 is different --- libavcodec/ffv1dec.c | 14 +- libavcodec/ffv1enc.c | 15 +-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 94bd60ad2b..9f26d48b03 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -594,7 +594,10 @@ static int read_header(FFV1Context *f) if (!f->transparency && !f->chroma_planes) { if (f->avctx->bits_per_raw_sample <= 8) f->avctx->pix_fmt = AV_PIX_FMT_GRAY8; -else if (f->avctx->bits_per_raw_sample == 10) { +else if (f->avctx->bits_per_raw_sample == 9) { +f->packed_at_lsb = 1; +f->avctx->pix_fmt = AV_PIX_FMT_GRAY9; +} else if (f->avctx->bits_per_raw_sample == 10) { f->packed_at_lsb = 1; f->avctx->pix_fmt = AV_PIX_FMT_GRAY10; } else if (f->avctx->bits_per_raw_sample == 12) { @@ -645,6 +648,7 @@ static int read_header(FFV1Context *f) f->packed_at_lsb = 1; switch(16 * f->chroma_h_shift + f->chroma_v_shift) { case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUV444P10; break; +case 0x01: f->avctx->pix_fmt = AV_PIX_FMT_YUV440P10; break; case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUV422P10; break; case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUV420P10; break; } @@ -659,9 +663,17 @@ static int read_header(FFV1Context *f) f->packed_at_lsb = 1; switch(16 * f->chroma_h_shift + f->chroma_v_shift) { case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUV444P12; break; +case 0x01: f->avctx->pix_fmt = AV_PIX_FMT_YUV440P12; break; case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUV422P12; break; case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUV420P12; break; } +} else if (f->avctx->bits_per_raw_sample == 14 && !f->transparency) { +f->packed_at_lsb = 1; +switch(16 * f->chroma_h_shift + f->chroma_v_shift) { +case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUV444P14; break; +case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUV422P14; break; +case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUV420P14; break; +} } else if (f->avctx->bits_per_raw_sample == 16 && !f->transparency){ f->packed_at_lsb = 1; switch(16 * f->chroma_h_shift + f->chroma_v_shift) { diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 262821abab..f0f9eaba79 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -559,6 +559,7 @@ FF_ENABLE_DEPRECATION_WARNINGS s->plane_count = 3; switch(avctx->pix_fmt) { +case AV_PIX_FMT_GRAY9: case AV_PIX_FMT_YUV444P9: case AV_PIX_FMT_YUV422P9: case AV_PIX_FMT_YUV420P9: @@ -569,6 +570,7 @@ FF_ENABLE_DEPRECATION_WARNINGS s->bits_per_raw_sample = 9; case AV_PIX_FMT_GRAY10: case AV_PIX_FMT_YUV444P10: +case AV_PIX_FMT_YUV440P10: case AV_PIX_FMT_YUV420P10: case AV_PIX_FMT_YUV422P10: case AV_PIX_FMT_YUVA444P10: @@ -578,11 +580,17 @@ FF_ENABLE_DEPRECATION_WARNINGS s->bits_per_raw_sample = 10; case AV_PIX_FMT_GRAY12: case AV_PIX_FMT_YUV444P12: +case AV_PIX_FMT_YUV440P12: case AV_PIX_FMT_YUV420P12: case AV_PIX_FMT_YUV422P12: -s->packed_at_lsb = 1; if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample) s->bits_per_raw_sample = 12; +case AV_PIX_FMT_YUV444P14: +case AV_PIX_FMT_YUV420P14: +case AV_PIX_FMT_YUV422P14: +if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample) +s->bits_per_raw_sample = 14; +s->packed_at_lsb = 1; case AV_PIX_FMT_GRAY16: case AV_PIX_FMT_YUV444P16: case AV_PIX_FMT_YUV422P16: @@ -1343,10 +1351,13 @@ AVCodec ff_ffv1_encoder = { AV_PIX_FMT_GRAY16,AV_PIX_FMT_GRAY8,
[FFmpeg-devel] [PATCH 5/7] avcodec/ffv1enc: support of 1024 slices
FFV1 decoder supports up to 1024 slices but FFV1 encoder permits only 961 (31x31) slices. This patch permits to create 1024 (32x32) slices. From 494115e12981a9c54bf04b63a13ef50364349de2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 10:49:24 +0100 Subject: [PATCH 5/7] avcodec/ffv1enc: support of 1024 slices Decoder already accepts 1024 slices but encoder was limited to 961 slices --- libavcodec/ffv1enc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 16987117d8..262821abab 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -872,7 +872,7 @@ FF_ENABLE_DEPRECATION_WARNINGS s->num_v_slices = FFMIN(s->num_v_slices, max_v_slices); -for (; s->num_v_slices < 32; s->num_v_slices++) { +for (; s->num_v_slices <= 32; s->num_v_slices++) { for (s->num_h_slices = s->num_v_slices; s->num_h_slices < 2*s->num_v_slices; s->num_h_slices++) { int maxw = (avctx->width + s->num_h_slices - 1) / s->num_h_slices; int maxh = (avctx->height + s->num_v_slices - 1) / s->num_v_slices; -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 4/7] avcodec/ffv1enc: prevent encoder to create non-lossless streams with some chroma subsamplings
When all luma samples for a chroma sample are not in the same slice, resulting bitstream is valid but the compression then decompression is not lossless. ffmpeg -y -f lavfi -i mandelbrot=s=1925x1080 -vf format=yuv411p -vframes 1 -c ffv1 -slices 16 a.mkv ffmpeg -y -f lavfi -i mandelbrot=s=1925x1080 -vf format=yuv411p -vframes 1 -f framemd5 source.md5 ffmpeg -y -i a.mkv -vframes 1 -f framemd5 a.mkv.md5 Framemd5 are not same. For e.g. 4:1:1 and 4 horizontal slices (-slices 16), we have 1920-1921-1922-1923-1924 1926-1927-1928 1931-1932 1936 OK 1925 1929-1930 1933-1934-1935 NOK This patch is an hotfix for preventing the encoder to create such stream. Note that it blocks more than needed (e.g, all the listed widths above except multiples of 16 are blocked) as I didn't find the exact pattern that makes the conversion NOK, the hotfix is definitely not nice and I would welcome another proposal. From 954817ec1f78396d04383a1ea19aec7739fdd7fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 10:41:10 +0100 Subject: [PATCH 4/7] avcodec/ffv1enc: prevent encoder to create non-lossless streams with some chroma subsamplings Some combinations having chroma subsampling are not lossless with current code --- libavcodec/ffv1enc.c | 4 1 file changed, 4 insertions(+) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 51aa8c2898..16987117d8 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -876,6 +876,10 @@ FF_ENABLE_DEPRECATION_WARNINGS for (s->num_h_slices = s->num_v_slices; s->num_h_slices < 2*s->num_v_slices; s->num_h_slices++) { int maxw = (avctx->width + s->num_h_slices - 1) / s->num_h_slices; int maxh = (avctx->height + s->num_v_slices - 1) / s->num_v_slices; +if (s->chroma_h_shift && (avctx->width % (s->num_h_slices << s->chroma_h_shift))) // Hotfix: some combinations having chroma subsampling are not lossless with current code, e.g. width of 1919 with 422 subsampling and 2 horizontal slices, or width of 1921 with 422 subsampling and 2 horizontal slices +continue; +if (s->chroma_v_shift && (avctx->height % (s->num_v_slices << s->chroma_v_shift))) // Hotfix: some combinations having chroma subsampling are not lossless with current code, e.g. width of 1919 with 422 subsampling and 2 horizontal slices, or width of 1921 with 422 subsampling and 2 horizontal slices +continue; if (s->num_h_slices > max_h_slices || s->num_v_slices > max_v_slices) continue; if (maxw * maxh * (int64_t)(s->bits_per_raw_sample+1) * plane_count > 8<<24) -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/7] avcodec/ffv1enc: prevent encoder to create buggy streams with small frame sizes
When there is 1 pixel per slice for the first half of slices, the encoder creates buggy slices. Example: ffmpeg -f lavfi -i mandelbrot=s=8x8 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=9x9 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=10x10 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=11x11 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=12x12 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv then for each file: ffmpeg -i a.mkv [ffv1 @ 01977d8ee240] bytestream end mismatching by -1 etc This patch is an hotfix for preventing the encoder to create such stream. From 1c78e038a9fb826c70d7b609430a92d5e98ce1de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 10:40:05 +0100 Subject: [PATCH 3/7] avcodec/ffv1enc: prevent encoder to create buggy streams with small frame sizes The first half of slices must have more than 1 pixel else encoder does not correctly handle slice_x and/or slice_y --- libavcodec/ffv1enc.c | 4 1 file changed, 4 insertions(+) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index ac8b715b74..51aa8c2898 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -864,6 +864,10 @@ FF_ENABLE_DEPRECATION_WARNINGS int plane_count = 1 + 2*s->chroma_planes + s->transparency; int max_h_slices = AV_CEIL_RSHIFT(avctx->width , s->chroma_h_shift); int max_v_slices = AV_CEIL_RSHIFT(avctx->height, s->chroma_v_shift); +while (max_h_slices && ((float)(avctx->width >> (s->chroma_h_shift))) / max_h_slices <= 1.5) // Hotfix: the first half of slices must have more than 1 pixel else encoder does not correctly handle slice_x +max_h_slices--; +while (max_v_slices && ((float)(avctx->height >> (s->chroma_v_shift))) / max_v_slices <= 1.5) // Hotfix: the first half of slices must have more than 1 pixel else encoder does not correctly handle slice_y +max_v_slices--; s->num_v_slices = (avctx->width > 352 || avctx->height > 288 || !avctx->slices) ? 2 : 1; s->num_v_slices = FFMIN(s->num_v_slices, max_v_slices); -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder
There is a message when coder type is forced to a value not chosen by user, but no message when version is forced to a value not chosen by user. This patch adds such message for more coherency in the messages, and the user is informed that the command is not fully respected. ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv Before: [ffv1 @ 02492CD69B40] bits_per_raw_sample > 8, forcing range coder After: [ffv1 @ 01A6E404A780] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01A6E404A780] bits_per_raw_sample > 8, forcing range coder From 49db6049fa50976683fc651cf180ab8c7428225e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 10:37:46 +0100 Subject: [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder --- libavcodec/ffv1enc.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index d71d952c6d..ac8b715b74 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -509,7 +509,7 @@ static av_cold int encode_init(AVCodecContext *avctx) { FFV1Context *s = avctx->priv_data; const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); -int i, j, k, m, ret; +int i, j, k, m, ret, oldversion; if ((ret = ff_ffv1_common_init(avctx)) < 0) return ret; @@ -534,6 +534,7 @@ static av_cold int encode_init(AVCodecContext *avctx) } s->version = avctx->level; } +oldversion = s->version; if (s->ec < 0) { s->ec = (s->version >= 3); @@ -679,6 +680,11 @@ FF_ENABLE_DEPRECATION_WARNINGS av_assert0(s->bits_per_raw_sample >= 8); if (s->bits_per_raw_sample > 8) { +if (oldversion >= 0 && oldversion != s->version) { +av_log(avctx, AV_LOG_INFO, +"bits_per_raw_sample > 8, forcing version 1\n"); +oldversion = s->version; +} if (s->ac == AC_GOLOMB_RICE) { av_log(avctx, AV_LOG_INFO, "bits_per_raw_sample > 8, forcing range coder\n"); -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/7] avcodec/ffv1enc: prevent encoder to create buggy streams with small frame sizes
When there is 1 pixel per slice for the first half of slices, the encoder creates buggy slices. Example: ffmpeg -f lavfi -i mandelbrot=s=8x8 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=9x9 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=10x10 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=11x11 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv ffmpeg -f lavfi -i mandelbrot=s=12x12 -vf format=yuv444p -t 1 -c ffv1 -coder 1 -context 0 -g 1 -level 3 -slices 64 -slicecrc 1 a.mkv then for each file: ffmpeg -i a.mkv [ffv1 @ 01977d8ee240] bytestream end mismatching by -1 etc This patch is an hotfix for preventing the encoder to create such stream. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder
There is a message when coder type is forced to a value not chosen by user, but no message when version is forced to a value not chosen by user. This patch adds such message for more coherency in the messages, and the user is informed that the command is not fully respected. ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv Before: [ffv1 @ 02492CD69B40] bits_per_raw_sample > 8, forcing range coder After: [ffv1 @ 01A6E404A780] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 01A6E404A780] bits_per_raw_sample > 8, forcing range coder ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/7] avcodec/ffv1dec: add missing error messages when a frame is invalid
A buggy file (before the patch preventing such issue is applied): ffmpeg -y -f lavfi -i mandelbrot=s=31x31 -vframes 1 -c ffv1 -slices 961 a.mkv Then with: ffmpeg -y -f lavfi -i mandelbrot=s=31x31 -vframes 1 source.jpg ffmpeg -y -i a.mkv a.jpg There is no error message despite the fact the stream was not correctly decoded (a.jpg is not like source.jpg), but user is not informed that the decoding was not good. This patch adds error message to the output when relevant. From 04f7275bdefe56ca2ff5d175de6e392f60c31bc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 7 Mar 2018 10:36:36 +0100 Subject: [PATCH 1/7] avcodec/ffv1dec: add missing error messages when a frame is invalid --- libavcodec/ffv1dec.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 3d2ee2569f..94bd60ad2b 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -296,6 +296,7 @@ static int decode_slice(AVCodecContext *c, void *arg) if (decode_slice_header(f, fs) < 0) { fs->slice_x = fs->slice_y = fs->slice_height = fs->slice_width = 0; fs->slice_damaged = 1; +av_log(f->avctx, AV_LOG_ERROR, "Invalid content found while parsing slice header\n"); return AVERROR_INVALIDDATA; } } @@ -432,8 +433,10 @@ static int read_extra_header(FFV1Context *f) if (f->version > 2) { c->bytestream_end -= 4; f->micro_version = get_symbol(c, state, 0); -if (f->micro_version < 0) +if (f->micro_version < 0) { +av_log(f->avctx, AV_LOG_ERROR, "Invalid micro_version in global header\n"); return AVERROR_INVALIDDATA; +} } f->ac = get_symbol(c, state, 0); @@ -759,11 +762,15 @@ static int read_header(FFV1Context *f) fs->slice_width = fs->slice_width / f->num_h_slices - fs->slice_x; fs->slice_height = fs->slice_height / f->num_v_slices - fs->slice_y; if ((unsigned)fs->slice_width > f->width || -(unsigned)fs->slice_height > f->height) +(unsigned)fs->slice_height > f->height) { +av_log(f->avctx, AV_LOG_ERROR, "Invalid content found while parsing header\n"); return AVERROR_INVALIDDATA; +} if ( (unsigned)fs->slice_x + (uint64_t)fs->slice_width > f->width -|| (unsigned)fs->slice_y + (uint64_t)fs->slice_height > f->height) +|| (unsigned)fs->slice_y + (uint64_t)fs->slice_height > f->height) { +av_log(f->avctx, AV_LOG_ERROR, "Invalid content found while parsing header\n"); return AVERROR_INVALIDDATA; +} } for (i = 0; i < f->plane_count; i++) { -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 0/7] Hardening FFV1
A bunch of independent patches related to FFV1 encoder and decoder, fixing some issues found during development of FFV1 regression tests (plan is to use a part of these regression tests in FATE). Some of the patches prevent FFmpeg to create buggy files, without fixing the issue. My point of view is that I would prefer to have FATE tests before trying to fix these issues, in order to avoid the risk of injecting bugs when patching the actual issue, to be debated. Anyway, these hotfix patches could be discarded without preventing other patches to be merged. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB 12-bit packed decoding
On 08/02/2018 11:28, Jerome Martinez wrote: Currently RGB and RGBA 12-bit are supported by DPX decoder only if component values are padded (packing "Filled to 32-bit words, method A"). This patch adds decoding of RGB and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). As I have no file with line boundaries not aligned on 32-bit, I can not have good tests about the stride computing (so code about non aligned boundaries is theory) so I preferred to limit risks by decoding only if line boundaries are aligned on 32-bit words: - 8 pixels for RGB (8 pixels x 3 components x 12 bits = 288 bits = 9 x 32-bit words) - 2 pixels for RGBA (2 pixels x 4 components x 12 bits = 3 x 32-bit words) I think Little Endian parsing is fine thanks to the generic code about Big vs Little endian but I have no Little Endian test file so I also limited the decoding to Big Endian files. Would be happy to check with cases I was not able to check if someone provides files. I kept "Packing to 16bit required\n" message but interested in any suggestion about a better wording due to exceptions. My test files: https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGB_12_Packed_BE Regression tests done on 12-bit content "Filled to 32-bit words, method A" in: https://samples.ffmpeg.org/image-samples/dpx_samples.zip Looks like the previous version of the patch is not included due to the debate about RGBA. Please consider to include this modified patch, which has the RGBA part commented (I could remove the commented lines if you prefer). From 28316686fed140279494d8c94e4f6bb5707e9ac0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Thu, 8 Feb 2018 09:22:08 +0100 Subject: [PATCH] avcodec/dpx: Support for RGB 12-bit packed decoding Limited to widths multiple of 8 (RGB) due to lack of test files for such corner case This partially fixes ticket #5639 --- libavcodec/dpx.c | 69 +--- 1 file changed, 66 insertions(+), 3 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 1aa2cbd1c8..7e2a6fb779 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -65,6 +65,38 @@ static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf, return *lbuf & 0x3FF; } +static uint16_t read12in32(const uint8_t **ptr, uint32_t * lbuf, + int * n_datum, int is_big) +{ +if (*n_datum) +(*n_datum)--; +else { +*lbuf = read32(ptr, is_big); +*n_datum = 7; +} + +switch (*n_datum){ +case 7: return *lbuf & 0xFFF; +case 6: return (*lbuf >> 12) & 0xFFF; +case 5: { +uint32_t c = *lbuf >> 24; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 8; +return c & 0xFFF; +} +case 4: return (*lbuf >> 4) & 0xFFF; +case 3: return (*lbuf >> 16) & 0xFFF; +case 2: { +uint32_t c = *lbuf >> 28; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 4; +return c & 0xFFF; +} +case 1: return (*lbuf >> 8) & 0xFFF; +default: return *lbuf >> 20; +} +} + static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, @@ -201,10 +233,29 @@ static int decode_frame(AVCodecContext *avctx, break; case 12: if (!packing) { -av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); -return -1; +int tested = 0; +if (endian && descriptor == 50 && (avctx->width%8) == 0) // Little endian and widths not a multiple of 8 need tests +tested = 1; +//if (endian && descriptor == 51 && (avctx->width%2) == 0) // Little endian and widths not a multiple of 2 need tests +//tested = 1; +if (!tested) { +av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); +return -1; +} +} +stride = avctx->width * elements; +if (packing) +stride *= 2; +else { +stride *= 3; // 12 bits are 1.5 byte so multiplied by 3 then divided by 2 +if (stride % 8) { +// Align to 32-bit boundaries (not tested) +stride /= 8; +stride++; +stride *= 8; +} +stride /= 2; } -stride = 2 * avctx->width * elements; break; case 16: stride = 2 * avctx->width * elements; @@ -349,6 +400,7 @@ static int decode_frame(AVCodecContext *avctx, (uint16_t*)ptr[2],
[FFmpeg-devel] [PATCH] avcodec/ffv1: Support for GBRAP10 and GBRAP12
Add support for 10- and 12-bit/component RGB with Alpha encoding and decoding in FFV1. Benched with START/STOP_TIMER around "for (x = 0; x < w; x++)" part during decoding, before the previous patch and with the previous patch + this patch, no obvious impact (+/-1%), e.g. with 1 second of gbrp12 4K content: ffmpeg -i a.mkv -f framemd5 a.framemd5 -y 325549 UNITS in decode_rgb_frame, 130899 runs, 173 skips frame= 24 fps=1.3 q=-0.0 Lsize= 2kB time=00:00:01.00 bitrate= 17.1kbits/s speed=0.0544x The 2 deleted lines are the ones I inadvertently kept after testing in my previous patch, they are actually never used. Some input test files used for testing (input file framemd5 = FFV1 file framemd5): https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2392/converted_image_gets_skewed.dpx https://samples.ffmpeg.org/image-samples/dpx_samples.zip For reference, this leads to the following array of supported pix_fmt (underscore means not supported): Y : 8/_/10/12/__/16 YA : 8/_/__/__/__/__ YUV 420: 8/9/10/12/__/16 YUVA 420: 8/9/10/__/__/16 YUV 422: 8/9/10/12/__/16 YUVA 422: 8/9/10/__/__/16 YUV 444: 8/9/10/12/__/16 YUVA 444: 8/9/10/__/__/16 RGB : 8/9/10/12/14/16 RGBA : 8/_/10/12/__/16 and 8-bit for YUV 410/411/440 it could be interesting for coherency in the listed supported pix_fmt to fill some gaps when corresponding pix_fmt exists in FFmpeg e.g. YUVA 12-bit, GRAY9 or YA16 From 3d24b30f2f23a2624e00333911e82fb48cf6d35c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Wed, 14 Feb 2018 08:39:15 +0100 Subject: [PATCH] avcodec/ffv1: Support for GBRAP10 and GBRAP12 --- libavcodec/ffv1dec.c | 4 libavcodec/ffv1dec_template.c | 4 +--- libavcodec/ffv1enc.c | 3 +++ libavcodec/ffv1enc_template.c | 2 +- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 923b79f3ab..3d2ee2569f 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -688,8 +688,12 @@ static int read_header(FFV1Context *f) f->avctx->pix_fmt = AV_PIX_FMT_GBRP9; else if (f->avctx->bits_per_raw_sample == 10 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP10; +else if (f->avctx->bits_per_raw_sample == 10 && f->transparency) +f->avctx->pix_fmt = AV_PIX_FMT_GBRAP10; else if (f->avctx->bits_per_raw_sample == 12 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP12; +else if (f->avctx->bits_per_raw_sample == 12 && f->transparency) +f->avctx->pix_fmt = AV_PIX_FMT_GBRAP12; else if (f->avctx->bits_per_raw_sample == 14 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP14; else if (f->avctx->bits_per_raw_sample == 16 && !f->transparency) { diff --git a/libavcodec/ffv1dec_template.c b/libavcodec/ffv1dec_template.c index a25b3384f1..f8a42a6d44 100644 --- a/libavcodec/ffv1dec_template.c +++ b/libavcodec/ffv1dec_template.c @@ -155,7 +155,7 @@ static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[4], int w, int if (lbd) *((uint32_t*)(src[0] + x*4 + stride[0]*y)) = b + ((unsigned)g<<8) + ((unsigned)r<<16) + ((unsigned)a<<24); -else if (sizeof(TYPE) == 4) { +else if (sizeof(TYPE) == 4 || transparency) { *((uint16_t*)(src[0] + x*2 + stride[0]*y)) = g; *((uint16_t*)(src[1] + x*2 + stride[1]*y)) = b; *((uint16_t*)(src[2] + x*2 + stride[2]*y)) = r; @@ -165,8 +165,6 @@ static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[4], int w, int *((uint16_t*)(src[0] + x*2 + stride[0]*y)) = b; *((uint16_t*)(src[1] + x*2 + stride[1]*y)) = g; *((uint16_t*)(src[2] + x*2 + stride[2]*y)) = r; -if (transparency) -*((uint16_t*)(src[3] + x*2 + stride[3]*y)) = a; } } } diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 49b8d590a3..d71d952c6d 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -648,9 +648,11 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!avctx->bits_per_raw_sample) s->bits_per_raw_sample = 9; case AV_PIX_FMT_GBRP10: +case AV_PIX_FMT_GBRAP10: if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample) s->bits_per_raw_sample = 10; case AV_PIX_FMT_GBRP12: +case AV_PIX_FMT_GBRAP12: if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample) s->bits_per_raw_sample = 12; case AV_PIX_FMT_GBRP14: @@ -1326,6 +1328,7 @@ AVCodec ff_ffv1_encoder = { AV_PIX_FMT_YUVA444P9, AV_PIX_FMT_YUVA422P9, AV_PIX_FMT_YUVA420P9, AV_PIX_FMT_GRAY16,AV_PIX_FMT_GRAY8, AV_PIX_FMT_GBRP9, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12,AV_PIX_FMT_GBRP14, +AV_PIX_FMT_GBRAP10, AV_PIX_FMT_GBRAP12,
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
On 12/02/2018 22:37, Carl Eugen Hoyos wrote: 2018-02-12 20:47 GMT+01:00 Jerome Martinez <jer...@mediaarea.net>: https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx is indicated by the person who provided it as with DPX alpha channel used for actually storing infrared Thank you! This sample appears to confirm that GraphicsMagick is right and FFmpeg is buggy. To avoid creating more incorrect (ffv1) encodes, I (strongly) suggest to first fix this issue and commit your patch to support more bit-depths but if you disagree, please feel free to commit! Sorry but I don't get it: the patch in this thread is totally separated from the alpha channel meaning issue. What should I "commit" about which "issue"? The only issues I have for the moment are that 12-bit padded DPX is supported by FFmpeg but not 12-bit packed DPX (the patch solves it), and that FFV1 support is with e.g. 12-bit YUVA but not 12-bit RGBA (I'll send a patch tomorrow for that, separated issue). I am not against continuing the discussion about the alpha channel meaning issue in a global manner, but I wish it is not blocking for this patch inclusion (I already sent the modified patch in order to fix the issues you indicated), as this patch does not create something new compared to what already exists (RGBA 8-bit, 16-bit, 10-bit padded, 12-bit padded, are already parsed by FFmpeg DPX parser) and the patch may be useful for people using "correctly" the alpha channel as they do for the other flavors of DPX, as well for people using RGB 12-bit packed. Let me know if I should split the patch in 2 (RGB part, RGBA part), so at least the RGB one could be included, as it is not related with any "alpha channel" stuff. There is also this sample though: http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2392/ The only solution I can think of is to change the semantics of the fourth dpx layer by default and to add an option (to the decoder) that allows using the current semantics that defaults to "auto" reading the encoder value. IMO having the default everywhere as currently set is not "buggy", but a global (not limited to DPX) option could be interesting for setting the semantics of the channel flagged as "alpha", because people may already use DPX or FFV1 (FFV1 supports RGBA 8-bit or YUVA 16-bit for a long time, nothing new) or any other format with alpha channel not having the "right" semantics (and whatever is the format, it is impossible to know how it is used) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
On 09/02/2018 11:39, Carl Eugen Hoyos wrote: I think the question is out of topic for this patch proposal, as this question is global for all flavors of DPX (also the ones already supported by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A, here I just add the support for packed content, without adding anything else about alpha support in FFmpeg, this patch changes nothing about alpha support in FFmpeg and/or DPX) and the need for validating such patch is about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A being already available) and not for the content itself. Sorry, I thought you had access to such a sample and we would just have to test it, no? https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx is indicated by the person who provided it as with DPX alpha channel used for actually storing infrared ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
On 09/02/2018 12:20, Kieran O Leary wrote: Hi both, I'm just wondering if the past duration errors in my earlier email mean anything,or should they be ignored? They are independent from the patch, e.g.: https://stackoverflow.com/questions/30782771/what-does-past-duration-x-xxx-too-large-mean ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
On 09/02/2018 11:32, Carl Eugen Hoyos wrote: [...] please mention ticket #5639 in the commit message. I should definitely have indicated this source instead of my copy, my mistake. Commit message extended with it. FYI I am running tests on several DPX flavors (to FFV1 and back to DPX, checking that framemd5 is same in order to be sure the lossless compression is really lossless), including the ones in this ticket, I'll post a message in this ticket when I finish and all files are well supported in FFmpeg. On 09/02/2018 11:39, Carl Eugen Hoyos wrote: Sorry, I thought you had access to such a sample and we would just have to test it, no? I use to stay on a technical side, if the DPX header says alpha I map to alpha (no way to differentiate from an illegitimate content, technically speaking), whatever are the hacks used by users (my goal is to store content with FFV1 in order to save disk space then convert back to DPX when needed, so such hack are the user problem and are not visible by FFmpeg). Sometimes I have contact with the source provider, sometimes not. I think some of the files I get are hacked with infrared instead of alpha, if you are interested in a sample file with infrared-hacked content for sure I'll ask if it is the case for some files I have. But not blocking for the patch IMO (especially because the files I am thinking to are not 12-bit) One more minor issue: +} +else { This is getting very common now (do you know why?), Different projects, different C style standards, so sometimes some mistakes when people work on several projects. (here, it was due to automatic edition from a code editor having a different standard by default, and I missed it when I reviewed the style) please fix it. Fixed, updated patch attached. From 986379350bc788701e39e2d6f9fce55b61b5fbb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Thu, 8 Feb 2018 09:22:08 +0100 Subject: [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding Limited to widths multiple of 8 (RGB) and 2 (RGBA) due to lack of test files for such corner case This partially fixes ticket #5639 --- libavcodec/dpx.c | 69 +--- 1 file changed, 66 insertions(+), 3 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 1aa2cbd1c8..7535de6c23 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -65,6 +65,38 @@ static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf, return *lbuf & 0x3FF; } +static uint16_t read12in32(const uint8_t **ptr, uint32_t * lbuf, + int * n_datum, int is_big) +{ +if (*n_datum) +(*n_datum)--; +else { +*lbuf = read32(ptr, is_big); +*n_datum = 7; +} + +switch (*n_datum){ +case 7: return *lbuf & 0xFFF; +case 6: return (*lbuf >> 12) & 0xFFF; +case 5: { +uint32_t c = *lbuf >> 24; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 8; +return c & 0xFFF; +} +case 4: return (*lbuf >> 4) & 0xFFF; +case 3: return (*lbuf >> 16) & 0xFFF; +case 2: { +uint32_t c = *lbuf >> 28; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 4; +return c & 0xFFF; +} +case 1: return (*lbuf >> 8) & 0xFFF; +default: return *lbuf >> 20; +} +} + static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, @@ -201,10 +233,29 @@ static int decode_frame(AVCodecContext *avctx, break; case 12: if (!packing) { -av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); -return -1; +int tested = 0; +if (endian && descriptor == 50 && (avctx->width%8) == 0) // Little endian and widths not a multiple of 8 need tests +tested = 1; +if (endian && descriptor == 51 && (avctx->width%2) == 0) // Little endian and widths not a multiple of 2 need tests +tested = 1; +if (!tested) { +av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); +return -1; +} +} +stride = avctx->width * elements; +if (packing) +stride *= 2; +else { +stride *= 3; // 12 bits are 1.5 byte so multiplied by 3 then divided by 2 +if (stride % 8) { +// Align to 32-bit boundaries (not tested) +stride /= 8; +stride++; +stride *= 8; +} +stride /= 2; } -stride = 2 * avctx->width * elements; break; case 16: stride = 2 * avctx->width * elements; @@ -349,6 +400,7 @@ static int decode_frame(AVCodecContext *avctx,
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
On 09/02/2018 11:15, Carl Eugen Hoyos wrote: Sorry, I see a line was missing in my first post, the one with the link to the RGBA test file. Test file for RGBA 12-bit packed: https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE Is this a sample from a four-channel film scanner (with infrared channel)? I think the question is out of topic for this patch proposal, as this question is global for all flavors of DPX (also the ones already supported by FFmpeg, i.e. FFmpeg already supports RGBA 12-bit filled with Method A, here I just add the support for packed content, without adding anything else about alpha support in FFmpeg, this patch changes nothing about alpha support in FFmpeg and/or DPX) and the need for validating such patch is about sample files for RGBA 12-bit packed (RGBA 12-bit filled with method A being already available) and not for the content itself. If it is blocking for the patch inclusion, I could remove the line permitting RGBA Packed decoding. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
On 09/02/2018 02:19, Carl Eugen Hoyos wrote: 2018-02-08 11:28 GMT+01:00 Jerome Martinez <jer...@mediaarea.net>: Currently RGB and RGBA 12-bit are supported by DPX decoder only if component values are padded (packing "Filled to 32-bit words, method A"). This patch adds decoding of RGB and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). I suggest we wait with this patch until we verified if RGBA dpx decoding is broken (I suspect so, I believe you have a sample to verify). Sorry, I see a line was missing in my first post, the one with the link to the RGBA test file. Test file for RGBA 12-bit packed: https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGBA_12_Packed_BE Regression test done on checkerboard_1080p_nuke_bigendian_12bit_alpha.dpx and checkerboard_1080p_nuke_littleendian_12bit_alpha.dpx in https://samples.ffmpeg.org/image-samples/dpx_samples.zip As-is, we wouldn't do anybody a favour. Thank you, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding
Currently RGB and RGBA 12-bit are supported by DPX decoder only if component values are padded (packing "Filled to 32-bit words, method A"). This patch adds decoding of RGB and RGBA 12-bit with no padding (packing "Packed into 32-bit words"). As I have no file with line boundaries not aligned on 32-bit, I can not have good tests about the stride computing (so code about non aligned boundaries is theory) so I preferred to limit risks by decoding only if line boundaries are aligned on 32-bit words: - 8 pixels for RGB (8 pixels x 3 components x 12 bits = 288 bits = 9 x 32-bit words) - 2 pixels for RGBA (2 pixels x 4 components x 12 bits = 3 x 32-bit words) I think Little Endian parsing is fine thanks to the generic code about Big vs Little endian but I have no Little Endian test file so I also limited the decoding to Big Endian files. Would be happy to check with cases I was not able to check if someone provides files. I kept "Packing to 16bit required\n" message but interested in any suggestion about a better wording due to exceptions. My test files: https://github.com/MediaArea/RAWcooked-RegressionTestingFiles/tree/master/Formats/DPX/Flavors/RGB_12_Packed_BE Regression tests done on 12-bit content "Filled to 32-bit words, method A" in: https://samples.ffmpeg.org/image-samples/dpx_samples.zip From bf95371e3964e198e22dc545fc75706dedf9029b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Thu, 8 Feb 2018 09:22:08 +0100 Subject: [PATCH] avcodec/dpx: Support for RGB and RGBA 12-bit packed decoding Limited to widths multiple of 8 (RGB) and 2 (RGBA) due to lack of test files for such corner case --- libavcodec/dpx.c | 70 +--- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c index 1aa2cbd1c8..aaacd243c9 100644 --- a/libavcodec/dpx.c +++ b/libavcodec/dpx.c @@ -65,6 +65,38 @@ static uint16_t read10in32(const uint8_t **ptr, uint32_t * lbuf, return *lbuf & 0x3FF; } +static uint16_t read12in32(const uint8_t **ptr, uint32_t * lbuf, + int * n_datum, int is_big) +{ +if (*n_datum) +(*n_datum)--; +else { +*lbuf = read32(ptr, is_big); +*n_datum = 7; +} + +switch (*n_datum){ +case 7: return *lbuf & 0xFFF; +case 6: return (*lbuf >> 12) & 0xFFF; +case 5: { +uint32_t c = *lbuf >> 24; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 8; +return c & 0xFFF; +} +case 4: return (*lbuf >> 4) & 0xFFF; +case 3: return (*lbuf >> 16) & 0xFFF; +case 2: { +uint32_t c = *lbuf >> 28; +*lbuf = read32(ptr, is_big); +c |= *lbuf << 4; +return c & 0xFFF; +} +case 1: return (*lbuf >> 8) & 0xFFF; +default: return *lbuf >> 20; +} +} + static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, @@ -201,10 +233,29 @@ static int decode_frame(AVCodecContext *avctx, break; case 12: if (!packing) { -av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); -return -1; +int tested = 0; +if (endian && descriptor == 50 && (avctx->width%8) == 0) // Little endian and widths not a multiple of 8 need tests +tested = 1; +if (endian && descriptor == 51 && (avctx->width%2) == 0) // Little endian and widths not a multiple of 2 need tests +tested = 1; +if (!tested) { +av_log(avctx, AV_LOG_ERROR, "Packing to 16bit required\n"); +return -1; +} +} +stride = avctx->width * elements; +if (packing) +stride *= 2; +else { +stride *= 3; // 12 bits are 1.5 byte so multiplied by 3 then divided by 2 +if (stride % 8) { +// Align to 32-bit boundaries (not tested) +stride /= 8; +stride++; +stride *= 8; +} +stride /= 2; } -stride = 2 * avctx->width * elements; break; case 16: stride = 2 * avctx->width * elements; @@ -349,6 +400,7 @@ static int decode_frame(AVCodecContext *avctx, (uint16_t*)ptr[2], (uint16_t*)ptr[3]}; for (y = 0; y < avctx->width; y++) { +if (packing) { if (elements >= 3) *dst[2]++ = read16(, endian) >> 4; *dst[0] = read16(, endian) >> 4; @@ -357,6 +409,18 @@ static int decode_frame(AVCodecContext *avctx, *dst[1]++ = read16(, endian) >> 4; if (elements == 4) *dst[3]++ = read16(, endian) >> 4; +} +
Re: [FFmpeg-devel] [Cellar] [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16
On 03/02/2018 14:48, Michael Niedermayer wrote: I hope this will not reduce interrest in working on a improved 9-16bit mode in v4. I don't like to put politics in technical stuff, but here this is politics, and I think that blocking an easy improvement of FFV1 v3 encoding/decoding in FFmpeg (actually just using the current FFV1 v3, and also v1 actually, specification without having artificial limitations about bit depths for a specific color space, i.e. 16-bit support was already there for YUV before I work on FFV1) just for forcing people to do a completely different work (e.g. working on FFV1 v4) is not fair and IMO is not in the idea behind open source. IMO if interest for 9-16bit (side note: I don't see why 8-bit should not be in the range, some ideas I have for v4 are useful also for 8-bit) mode in v4 is reduced, that would only mean that v3 is already so great and/or just that people have no better idea right now, it is not bad, and both works (using v3 at the maximum of its possibilities and working on v4) are separate works. FYI criticism I saw about FFV1 is not compression ratio but speed (playing a 4K stream is just impossible on a "normal" machine, you need a very expensive CPU for that) so my plan is to focus on speed improvement in priority. It could be v4 stuff (incompatible changes), or v3 (encoder/decoder optimization), depending of what I find. [...] Last but not least, since almost two years now it's impossible to work on the development of FFV1 v4. It's always the wrong time and/or the wrong place every time I am doing something for this cause. Why? This is extremely frustrating. I want to understand this too. IMO v4 development should be more important than or at least equally to the v3 draft polishing and neither should block the other. Also politics, but I don't understand who is blocking v4 from your point of view. "impossible to work on v4"? Where? As far as I see 1 person (me) said his priority is having FFV1 v3 becoming a standard (so IETF related work) and widely used (so any bit depth for any supported color space in v3 because it is an easy demonstration that FFV1 is versatile without having to wait for v4 R, especially because v3 bitstream design is already pretty good and versatile) because this is what I need, and my personal case is not blocking anyone else for sending patches for either FFV1 specifications or FFmpeg code about v4. Eager to see patch proposals for v4 (and v3 if it does not break support of files in the wild), whoever feeling blocked with his patches should send patch proposals to either FFmpeg (code) or CELLAR mailing list (bitstream format). That said, I plan to add more pix_fmt support for FFV1 v3 (which is already a good format!) support in FFmpeg and some optimization ideas beside my work for IETF spec, with the hope we could speak about technical issues (e.g. more optimization hints or info about wrong code or warning about that it breaks the bitstream specs as currently written), leaving aside politics. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16
On 04/02/2018 15:00, Michael Niedermayer wrote: diff --git a/libavcodec/ffv1enc_template.c b/libavcodec/ffv1enc_template.c index b7eea0dd70..2763082540 100644 --- a/libavcodec/ffv1enc_template.c +++ b/libavcodec/ffv1enc_template.c @@ -122,8 +122,8 @@ static av_always_inline int RENAME(encode_line)(FFV1Context *s, int w, return 0; } -static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[3], -int w, int h, const int stride[3]) +static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[4], +int w, int h, const int stride[4]) { int x, y, p, i; const int ring_size = s->context_model ? 3 : 2; @@ -152,14 +152,18 @@ static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[3], r = (v >> 16) & 0xFF; a = v >> 24; } else if (packed) { -const uint16_t *p = ((const uint16_t*)(src[0] + x*6 + stride[0]*y)); +const uint16_t *p = ((const uint16_t*)(src[0] + x*(3 + s->transparency)*2 + stride[0]*y)); r = p[0]; g = p[1]; b = p[2]; +if (s->transparency) transparency should possibly be moved into a local variable Done, actually both s->transparency and (3 + s->transparency)*2 + a = p[3]; } else if (sizeof(TYPE) == 4) { g = *((const uint16_t *)(src[0] + x*2 + stride[0]*y)); b = *((const uint16_t *)(src[1] + x*2 + stride[1]*y)); r = *((const uint16_t *)(src[2] + x*2 + stride[2]*y)); +if (s->transparency) +a = *((const uint16_t *)(src[3] + x*2 + stride[2]*y)); ^ this looks wrong Stupid typo not seen as strides are same here, fixed (I reviewed again the patch, looks like it is the only one in the list of changes). Updated patch attached. also what speed effect do thes changes to the inner loop have ? I see same performance, which I expect because when I do performance profiling I see that the transformation part is taking less than 0.1% of the CPU time and I guess that the CPU prediction stuff does its job here. From 0b9501984a8ce95d6e88a448683fd0d9d1473a08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Thu, 1 Feb 2018 13:11:53 +0100 Subject: [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16 --- libavcodec/ffv1dec.c | 14 ++ libavcodec/ffv1dec_template.c | 9 +++-- libavcodec/ffv1enc.c | 16 ++-- libavcodec/ffv1enc_template.c | 14 ++ 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 5eadb6b158..923b79f3ab 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -336,14 +336,16 @@ static int decode_slice(AVCodecContext *c, void *arg) decode_plane(fs, p->data[0] + ps*x + y*p->linesize[0], width, height, p->linesize[0], 0, 2); decode_plane(fs, p->data[0] + ps*x + y*p->linesize[0] + 1, width, height, p->linesize[0], 1, 2); } else if (f->use32bit) { -uint8_t *planes[3] = { p->data[0] + ps * x + y * p->linesize[0], +uint8_t *planes[4] = { p->data[0] + ps * x + y * p->linesize[0], p->data[1] + ps * x + y * p->linesize[1], - p->data[2] + ps * x + y * p->linesize[2] }; + p->data[2] + ps * x + y * p->linesize[2], + p->data[3] + ps * x + y * p->linesize[3] }; decode_rgb_frame32(fs, planes, width, height, p->linesize); } else { -uint8_t *planes[3] = { p->data[0] + ps * x + y * p->linesize[0], +uint8_t *planes[4] = { p->data[0] + ps * x + y * p->linesize[0], p->data[1] + ps * x + y * p->linesize[1], - p->data[2] + ps * x + y * p->linesize[2] }; + p->data[2] + ps * x + y * p->linesize[2], + p->data[3] + ps * x + y * p->linesize[3] }; decode_rgb_frame(fs, planes, width, height, p->linesize); } if (fs->ac != AC_GOLOMB_RICE && f->version > 2) { @@ -694,6 +696,10 @@ static int read_header(FFV1Context *f) f->avctx->pix_fmt = AV_PIX_FMT_GBRP16; f->use32bit = 1; } +else if (f->avctx->bits_per_raw_sample == 16 && f->transparency) { +f->avctx->pix_fmt = AV_PIX_FMT_GBRAP16; +f->use32bit = 1; +} } else { av_log(f->avctx, AV_LOG_ERROR, "colorspace not supported\n"); return AVERROR(ENOSYS); diff --git a/libavcodec/ffv1dec_template.c b/libavcodec/ffv1dec_template.c index 37df766773..a25b3384f1 100644 ---
Re: [FFmpeg-devel] [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16
On 03/02/2018 00:10, Michael Niedermayer wrote: On Thu, Feb 01, 2018 at 01:43:00PM +0100, Jerome Martinez wrote: Add support for 16-bit/component RGB with Alpha encoding and decoding in FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding. Resulting bitstream was tested about lossless encoding/decoding by the compression from DPX to FFV1 then decompression from FFV1 to DPX, see commands below (resulting framemd5 hashes are all same). Resulting bitstream is decodable by another decoder (with same resulting framemd5 hash). Resulting bitstream passed through a conformance checker compared to current FFV1 specification IETF draft. About the patch: - some modified lines are not used (the ones not used when f->use32bit is 1), but it makes the code more coherent (especially because decode_rgb_frame signature is same for both 16-bit and 32-bit version) and prepares the support of RGBA with 10/12/14 bits/component. - GBRAP16 was chosen for decoding because GBRP16 is already used when no alpha, and the code is more prepared for planar pix_fmt when bit depth is 8. - "s->transparency = desc->nb_components == 4 || desc->nb_components == 2;" is a copy of a line a bit above about the detection of transparency, I preferred to reuse it as is even if "YA" 16-bit/component is not (yet) supported. FFmpeg commands used for tests: ./ffmpeg -i in.dpx -c:v ffv1 out.mkv ./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv ./ffmpeg -i out.mkv out.dpx ./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5 ./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5 ./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5 ./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5 Test file used (renamed to in.dpx): https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx I would prefer if the algorithm would be tuned to 16bit data before adding more formats to the encoder which require all decoders to support them. Dont you agree that this would be the better strategy ? ccing CELLAR. My remarks are the same as with RGB48 support (including that the compression performance of RGB48 so RGBA64 is already very good without touching on the algorithm, and IMO tuning should be for v4 for all bit depths), with addition that since the last debate on that on ffmpeg-devel there was no patch proposal so no consensus on CELLAR for limiting the specifications to what exists in FFmpeg implementation (so current consensus is that FFV1 specs are for all bit depths for all supported color spaces), and since the last debate FFV1 specs draft were sent to IETF tracker so it is close to the end. This patch is just adding the support of RGBA64 conforming to the current specs and without big changes (no complex stuff, just mapping to the right pix_fmt), and the current specs are the ones with very high chances to be the standard (up to now nobody suggested on CELLAR, the place for the spec, to limit the support to a set of bit depths / color spaces, and nobody suggested a tuning for some bit depths), with the main advantage that the specs are clear about all bit depths for all color spaces supported (it is good that it is generic). Will this patch be accepted after the IETF flags the current specs as stable if there is no changes on the wording about the support of all bit depths? on my side, I can not spend time on FFV1 v4 R (tuning and more) when I spend time with such blocking (for me) issue about v3 (i.e. agreement in specs draft on all bit depths for all supported color spaces but no agreement in practice on all bit depths for all supported color spaces). So for answering directly to the question, no I don't agree that changing v3 bitstream with specific tuning of the bitstream depending of the bit depth is a better strategy, actually this is the opposite (I think that the best strategy is to support as many bit depths as possible in implementations with current v3 specs and that all tuning should be in the version flagged as experimental, not the one flagged as stable even before working on IETF version, if we change a bitstream marked as stable we break the trust in the spec being stable, IMO any tuning of the bitstream should be done in v4, and any performance improvement without breaking the bitstream could be done after this patch without problem). Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] FFV1: make RGB48 support as non-experimental
On 05/01/2018 11:18, Jerome Martinez wrote: 1 year without RGB48 related patches, tested by a couple of users, tested with a FFV1 conformance checker, I suggest that FFV1 RGB48 support in FFmpeg does not mandate anymore the user to add " -strict experimental" on the command line during encoding. Sorry, I missed the GBRP16 part. I tested the encoding through GBRP16 pix_fmt (instead of RGB48 pix_fmt), and it works fine too. Additional patch for removing the need of " -strict experimental" for GBRP16 too. From 511e036499f716b14fed7ec07d2d8ccf18936444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Thu, 1 Feb 2018 13:15:54 +0100 Subject: [PATCH] avcodec/ffv1enc: mark RGB48 support as non-experimental Remove the 2nd mark, 1st mark was removed in 58e16a4 --- libavcodec/ffv1enc.c | 4 1 file changed, 4 deletions(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index c0c1558ffe..0778f84c9b 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -657,10 +657,6 @@ FF_ENABLE_DEPRECATION_WARNINGS s->chroma_planes = 1; if (s->bits_per_raw_sample >= 16) { s->use32bit = 1; -if (avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) { -av_log(avctx, AV_LOG_ERROR, "16bit RGB is experimental and under development, only use it for experiments\n"); -return AVERROR_INVALIDDATA; -} } s->version = FFMAX(s->version, 1); break; -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16
Add support for 16-bit/component RGB with Alpha encoding and decoding in FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding. Resulting bitstream was tested about lossless encoding/decoding by the compression from DPX to FFV1 then decompression from FFV1 to DPX, see commands below (resulting framemd5 hashes are all same). Resulting bitstream is decodable by another decoder (with same resulting framemd5 hash). Resulting bitstream passed through a conformance checker compared to current FFV1 specification IETF draft. About the patch: - some modified lines are not used (the ones not used when f->use32bit is 1), but it makes the code more coherent (especially because decode_rgb_frame signature is same for both 16-bit and 32-bit version) and prepares the support of RGBA with 10/12/14 bits/component. - GBRAP16 was chosen for decoding because GBRP16 is already used when no alpha, and the code is more prepared for planar pix_fmt when bit depth is >8. - "s->transparency = desc->nb_components == 4 || desc->nb_components == 2;" is a copy of a line a bit above about the detection of transparency, I preferred to reuse it as is even if "YA" 16-bit/component is not (yet) supported. FFmpeg commands used for tests: ./ffmpeg -i in.dpx -c:v ffv1 out.mkv ./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv ./ffmpeg -i out.mkv out.dpx ./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5 ./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5 ./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5 ./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5 Test file used (renamed to in.dpx): https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx Jérôme From 0e149afd7eadb1ec05cc79fff78337b2c543ad8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Thu, 1 Feb 2018 13:11:53 +0100 Subject: [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16 --- libavcodec/ffv1dec.c | 14 ++ libavcodec/ffv1dec_template.c | 6 +- libavcodec/ffv1enc.c | 16 ++-- libavcodec/ffv1enc_template.c | 10 +++--- 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 5eadb6b158..923b79f3ab 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -336,14 +336,16 @@ static int decode_slice(AVCodecContext *c, void *arg) decode_plane(fs, p->data[0] + ps*x + y*p->linesize[0], width, height, p->linesize[0], 0, 2); decode_plane(fs, p->data[0] + ps*x + y*p->linesize[0] + 1, width, height, p->linesize[0], 1, 2); } else if (f->use32bit) { -uint8_t *planes[3] = { p->data[0] + ps * x + y * p->linesize[0], +uint8_t *planes[4] = { p->data[0] + ps * x + y * p->linesize[0], p->data[1] + ps * x + y * p->linesize[1], - p->data[2] + ps * x + y * p->linesize[2] }; + p->data[2] + ps * x + y * p->linesize[2], + p->data[3] + ps * x + y * p->linesize[3] }; decode_rgb_frame32(fs, planes, width, height, p->linesize); } else { -uint8_t *planes[3] = { p->data[0] + ps * x + y * p->linesize[0], +uint8_t *planes[4] = { p->data[0] + ps * x + y * p->linesize[0], p->data[1] + ps * x + y * p->linesize[1], - p->data[2] + ps * x + y * p->linesize[2] }; + p->data[2] + ps * x + y * p->linesize[2], + p->data[3] + ps * x + y * p->linesize[3] }; decode_rgb_frame(fs, planes, width, height, p->linesize); } if (fs->ac != AC_GOLOMB_RICE && f->version > 2) { @@ -694,6 +696,10 @@ static int read_header(FFV1Context *f) f->avctx->pix_fmt = AV_PIX_FMT_GBRP16; f->use32bit = 1; } +else if (f->avctx->bits_per_raw_sample == 16 && f->transparency) { +f->avctx->pix_fmt = AV_PIX_FMT_GBRAP16; +f->use32bit = 1; +} } else { av_log(f->avctx, AV_LOG_ERROR, "colorspace not supported\n"); return AVERROR(ENOSYS); diff --git a/libavcodec/ffv1dec_template.c b/libavcodec/ffv1dec_template.c index 37df766773..2904a44112 100644 --- a/libavcodec/ffv1dec_template.c +++ b/libavcodec/ffv1dec_template.c @@ -107,7 +107,7 @@ static av_always_inline int RENAME(decode_line)(FFV1Context *s, int w, return 0; } -static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[3], int w, int h, int stride[3]) +static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[4], int w, int h, int stride[4]) { int x, y, p; TYPE *sample[4][2]; @@ -158,10 +158,14 @@ static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[3], int w, int *((uint16_t*)(src[0] + x*2 + stride[0]*y)) = g; *((uint16_t*)(src[1] + x*2 +
Re: [FFmpeg-devel] [Cellar] [PATCH] FFV1: make RGB48 support as non-experimental
On 07/01/2018 20:08, Michael Niedermayer wrote: [...] This is correct but i think misleading a bit the 17bit case this is about uses a seperate codepath. For your (FFmpeg) encoder and decoder. Not mine (same code path is used for 8/10/12/16/... bit RGB and YUV with my encoder and decoder). Again, we mix up bitstream specification and FFmpeg implementation. In the bitstream specifications, there is currently only one path (except for handling files in the wild which are not conform to the unique path, we take care of them for historical reasons). So if its enabled we generate new files that have never been generated before in some sense AFAIK - MediaConch conformance checker can check files up to 30 bits per component, and already implements the way it is written in spec for all supported bit depths, because the spec was flagged as stable whatever is the bit depth for a long time, without having anyone saying that 16 or 17-bit for RGB (reminder: it is already set as "stable" for YCbCr 16-bit) may have a different paths later for versions 0, 1, 3. If you change the bitstream of RGB48 or any stream with bitdepth <=30, you break it despite the fact it is validating correctly and would be a major issue for this tool (breaking trust about the tool or FFV1, as there would be 2 variants of FFV1 RGB48). - I have an early (not yet public, for testing the spec only for the moment) version of an encoder conforming to the spec for all bit depths up to 30-bit per component. - I have heard about other FFV1 encoders able to encode RGB48 If we change the bitstream in a future version, which i belive we should consider. Then we have an additional "old" version of the 17bit path that needs to be supported in implementations. question: How would you detect old version, from all encoders (not only FFmpeg)? there is nothing permitting that in the bitstream AFAIK (for v1: no micro_version; for v3: micro version >4 must be compatible with v4 as v4 is flagged stable). So it is "bitstream is in stone" now, for at least versions 0, 1, 3. Reason I speak about R with version 4 bitstream (I don't speak about FFmpeg), which is still flagged as experimental for all files. FFmpeg experimental flag is for the status of the encoder, not for the status of the bitstream (the bitstream for versions 0, 1, 3 is already considered stable for RGB48 and others) I think i added the flag check. The reason behind the check was so that the bitstream syntax can be changed without the need to support every iteration of the bitstream. I had hoped someone would fund research and testing of further improvments to the various fine details of higher bit depth encoding beyond 8bit. IIRC No further funding was provided, Noone worked on it as a volunteer, No changes where proposed ... This is planned on my side after FFV1 versions 0, 1, 3 are finalized. With version 4 (the experimental version). but it is possible only if we all are in sync and don't do something against the consensus, and as far I as I understand the consensus is that versions 0, 1, 3 are "bitstream is in stone" now (and actually from the beginning of CELLAR) based on the draft of FFV1 specifications, which is detailed before going to IETF but not changed. All the communication, at IETF and conferences, around FFV1 versions 0, 1, 3 is that the bitstream specification is stable, and discussing about breaking this consensus may harm FFV1 spread. But it wasnt intended as a "bitstream is in stone, encoder might mismatch the spec" at the time. I disagree: the idea behind CELLAR is that all encoders **must** match the spec, or there is misunderstanding to clarify. Else everyone does his own version of FFV1, and we can not work together on an unique lossless compression format. The spec is expected to be the reference (for versions 0, 1, 3 for the moment) and encoders are expected to match the spec. We have a draft spec now that does not limit bitdepth in any way, this may have been a mistakke but i dont see this as a big problem honestly. I do not propose to change this. But i would not oppose it if people want to change it If someone creates a 19bit per sample file based on the draft spec it also will not decode with most real world implementations. It will with my conformance checker. You can not know what is "real world", as a couple of encoders may be in house only. we still mix up bitstream specification and one implementation. The idea behind CELLAR is to have FFV1 a standardized video format, so we can not decide for 1 encoder to change the bitstream. We need to find a consensus on CELLAR mailing-list first. The consensus is what is written in the draft and there was no post on CELLAR mailing-list for limiting to some bit depths. There is no question that with a unlimited bitdepth, real implementations will never support some depths Whatever is the support from the encoders you know, you can not know what was
Re: [FFmpeg-devel] [PATCH] FFV1: make RGB48 support as non-experimental
On 06/01/2018 23:21, Michael Niedermayer wrote: On Sat, Jan 06, 2018 at 04:54:18PM +0100, Jerome Martinez wrote: On 06/01/2018 02:05, Michael Niedermayer wrote: ffv1enc.c |4 1 file changed, 4 deletions(-) acfc60c913b311b148f2eeef2d2d6ea9e37afcf7 0001-avcodec-ffv1enc-mark-RGB48-support-as-non-experiment.patch From 303f63fb7e6172fdb7de66da1f8a4006b79a535f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Fri, 5 Jan 2018 11:09:01 +0100 Subject: [PATCH] avcodec/ffv1enc: mark RGB48 support as non-experimental Resulting bitstream was tested with a conformance checker using the last draft of FFV1 specifications. But has the way this is stored been optimized ? Once its marked as non exerimental all future decoders must support the exact way. Although this is considered experimental in the encoder, the implementation adheres to the description in the specification. The bitstream specification does not provide a bitdepth limit so with the current draft of the specification, another FFV1 encoder could already encode 16-bit (and more) content. The work on the specification has been careful to not break compatibility with former drafts and at this point the FFV1 bitstream specification for versions 0, 1, and 3 should be considered already non-experimental for all bit depths. All current decoders should already support the exact way it is currently specified. To make a change in the specification at this point would have cascading consequences as we’d have to retract the part of the specification which states that micro_version 4 of version 3 is the first stable variant. Worse, it is impossible to indicate a bitstream change in FFV1 version 1, which permits RGB 16-bit content too, so it would be difficult for a decoder to detect a bitstream not conforming to the bitstream created by the current version of FFmpeg encoder. Are you not making this look alot more complex than it is ? Or maybe we talk about slightly different things here with the next version we can introduce any method of storing 16bit or 9-15 bit and we certainly do not support in the implementation all possible bit depths From what i remember I had always wanted to improve the way that more than 8bit is handled, not just 16. Although 16 is a bit special Consider this: If we improve get_context() in the next version for >8bit we still have to support 9-15 bit with the old definition if we now declare 16bit non experimental then we also must support that with an old get_context() in the decoder. the 16bit path is seperate from the lower bit depth. So this is an Additional codepath that we have to carry in the future isnt it smarter now that if we want to improve get_context() that we dont now extend what can be generated with the current get_context ? or are such current get_context() style files already widespread ? if so then its probably best to accept it and keep supporting it I am lost. Looks like we talk about 2 different subjects: FFV1 bitstream specifications and FFmpeg implementation. Let separate subjects: FFV1 bitstream specifications: Since at least 2012 [1] get_context (in the bitstream) is defined and flagged as stable for **all** bit depths for versions 0, 1, 3. It is still the case today [2]. There was a consensus on discussing about FFV1 bitstream on CELLAR mailing list There was a consensus for not changing the bitstream for version 0, 1, 3 so we can standardize it ASAP without breaking current implementations (reason we document bugs in the main encoder, but the idea is not to accept more changes) If the FFV1 bitstream for version 0, 1, or 3 must be changed in current stable version, it would be a major break in the consensus and it should be discussed on CELLAR list (in copy as they are concerned), but I doubt the consensus would be for breaking the bitstream compatibility as the discussion from day 0 on CELLAR is to document current bitstream without changing it for versions 0, 1, 3. The fact that there was no (publicly available) RGB48 encoder in the wild is not an excuse for breaking the compatibility with current draft of specifications. Same if someone decides to do an encoder for e.g. RGB3000. It is possible to change the bitstream with version 4 (experimental version), and it is a good place for changing get_context for 16-bit content (whatever is the color space, BTW). FFmpeg implementation: FFV1 YCbCr 16-bit is already flagged as non experimental, so there is already some non experimental 16-bit support in FFmpeg FFV1 encoder. 16-bit is not new and for RGB48 the actual encoding after RGB to YCbCr transformation is just 1 bit more (17-bit). FFmpeg experimental flag is for the status of the encoder, not for the status of the bitstream (the bitstream for versions 0, 1, 3 is already considered stable for RGB48 and others) FFV1 RGB48 support in FFmpeg is conform to FFV1 bitstream specifications. Optimizing FFmpeg f
Re: [FFmpeg-devel] [PATCH] FFV1: make RGB48 support as non-experimental
On 06/01/2018 02:05, Michael Niedermayer wrote: ffv1enc.c |4 1 file changed, 4 deletions(-) acfc60c913b311b148f2eeef2d2d6ea9e37afcf7 0001-avcodec-ffv1enc-mark-RGB48-support-as-non-experiment.patch From 303f63fb7e6172fdb7de66da1f8a4006b79a535f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Fri, 5 Jan 2018 11:09:01 +0100 Subject: [PATCH] avcodec/ffv1enc: mark RGB48 support as non-experimental Resulting bitstream was tested with a conformance checker using the last draft of FFV1 specifications. But has the way this is stored been optimized ? Once its marked as non exerimental all future decoders must support the exact way. Although this is considered experimental in the encoder, the implementation adheres to the description in the specification. The bitstream specification does not provide a bitdepth limit so with the current draft of the specification, another FFV1 encoder could already encode 16-bit (and more) content. The work on the specification has been careful to not break compatibility with former drafts and at this point the FFV1 bitstream specification for versions 0, 1, and 3 should be considered already non-experimental for all bit depths. All current decoders should already support the exact way it is currently specified. To make a change in the specification at this point would have cascading consequences as we’d have to retract the part of the specification which states that micro_version 4 of version 3 is the first stable variant. Worse, it is impossible to indicate a bitstream change in FFV1 version 1, which permits RGB 16-bit content too, so it would be difficult for a decoder to detect a bitstream not conforming to the bitstream created by the current version of FFmpeg encoder. It can no longer then be changed, so we need to be really sure the design is optimal first. Are we ? who has checked alternatives? what where the reasons why the alternatives were not choosen? for example consider get_context(), what it does with >8bit may or may not be optimal iam interrested to work on that in fact, ive a quite long and growing list of other volunteer jobs to do though ... bitdepths >8bit have been well-used for years since many of them have long been marked as non-experimental (for instance 10bit is frequently used with lossless encoding of broadcast media and video from analog tape sources). In my opinion get_context() is specified for all bitdpeths and non-experimental for FFV1 versions 0, 1, and 3 by the specification work and it should not be changed in these versions. For the encoder there may still be an opportunity to optimize while continuing to conform to the FFV1 versions 0, 1, and 3 bitstream specification, even if the encoder marks RGB48 as non-experimental. Additionally FFV1 version 4 or later could consider further optimization requesting a change in the FFV1 bitstream as version 4 has no stable micro_version and the entire version is in an experimental status. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] FFV1: make RGB48 support as non-experimental
On 05/01/2018 16:14, Tobias Rapp wrote: On 05.01.2018 11:18, Jerome Martinez wrote: 0001-FFV1-make-RGB48-support-as-non-experimental.patch From cd1bfe68a1a809700f25e593ac21ca3876d265ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Fri, 5 Jan 2018 11:09:01 +0100 Subject: [PATCH] FFV1: make RGB48 support as non-experimental make => mark I reused a commit text from history, both words words were in it. Anyway, attached is the updated .patch with the suggested subject line. BTW: common subject line format would be something like "avcodec/ffv1enc: mark RGB48 support as non-experimental" From 303f63fb7e6172fdb7de66da1f8a4006b79a535f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Fri, 5 Jan 2018 11:09:01 +0100 Subject: [PATCH] avcodec/ffv1enc: mark RGB48 support as non-experimental Resulting bitstream was tested with a conformance checker using the last draft of FFV1 specifications. --- libavcodec/ffv1enc.c | 4 1 file changed, 4 deletions(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 09df4c0c57..c0c1558ffe 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -630,10 +630,6 @@ FF_ENABLE_DEPRECATION_WARNINGS s->bits_per_raw_sample = 16; s->use32bit = 1; s->version = FFMAX(s->version, 1); -if (avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) { -av_log(avctx, AV_LOG_ERROR, "16bit RGB is experimental and under development, only use it for experiments\n"); -return AVERROR_INVALIDDATA; -} break; case AV_PIX_FMT_0RGB32: s->colorspace = 1; -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] FFV1: make RGB48 support as non-experimental
1 year without RGB48 related patches, tested by a couple of users, tested with a FFV1 conformance checker, I suggest that FFV1 RGB48 support in FFmpeg does not mandate anymore the user to add " -strict experimental" on the command line during encoding. From cd1bfe68a1a809700f25e593ac21ca3876d265ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Fri, 5 Jan 2018 11:09:01 +0100 Subject: [PATCH] FFV1: make RGB48 support as non-experimental Resulting bitstream was tested with a conformance checker using the last draft of FFV1 specifications. --- libavcodec/ffv1enc.c | 4 1 file changed, 4 deletions(-) diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index 09df4c0c57..c0c1558ffe 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -630,10 +630,6 @@ FF_ENABLE_DEPRECATION_WARNINGS s->bits_per_raw_sample = 16; s->use32bit = 1; s->version = FFMAX(s->version, 1); -if (avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) { -av_log(avctx, AV_LOG_ERROR, "16bit RGB is experimental and under development, only use it for experiments\n"); -return AVERROR_INVALIDDATA; -} break; case AV_PIX_FMT_0RGB32: s->colorspace = 1; -- 2.13.3.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/matroskaenc: Do not write 0 duration for subtitles
On 23/11/2017 20:02, John Stebbins wrote: [...] It's not practical to know the duration of the previous subtitle before writing it because you can't know it till you have seen this empty PGS. Once you've seen the empty PGS, it is often too late to properly interleave the previous PGS in the output file. I argued that on CELLAR mailing list (this is especially true in the case of live streaming) but did not find consensus about the need to have a different semantic for "clear" compared to "display empty subtitle", so for "clear" you have to use "display empty subtitle" thing as a workaround. It may change in the future. Your suggestion (0 means up to next frame) however works I believe. I.e. display the "empty" PGS up to the next frame. Right. I actually don't change the code in the patch, just the reason. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/matroskaenc: Do not write 0 duration for subtitles
On 12/11/2017 03:12, Carl Eugen Hoyos wrote: The matroska spec says blockduration == 0 means the frame is not a keyframe. Since all subtitles are "keyframes", 0 blockduration should not be written. As I understand from discussion on CELLAR mailing-list: - if is not expected to have a frame with block duration of 0, in a perfect world the previous frame should have the right duration and the player should hide the previous frame after blockduration of the previous frame. - as a workaround, it would make sense to put an empty frame with "unlimited" duration, so no blockduration, as implemented in the patch. The only issue is in the case someone puts a "not empty" frame with a duration of 0, this patch would change the behavior (from nothing displayed to the frame displayed) but as it is not expected to have a duration of 0 when something is displayed, this is not a real use case IMO. Suggestion of changes for this patch: - Remove the outdated part about specs. A duration of 0 is not coherent, that's all. - Add a comment (in the code?) stipulating that duration of 0 is a special case which means "up to the next frame". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/matroskaenc: Do not write 0 duration for subtitles
On 12/11/2017 03:12, Carl Eugen Hoyos wrote: The matroska spec says blockduration == 0 means the frame is not a keyframe. Since all subtitles are "keyframes", 0 blockduration should not be written. The issue is in the specifications: https://github.com/Matroska-Org/matroska-specification/pull/207 The patch should not be integrated as the reason is no more valid. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/matroskaenc: Do not write 0 duration for subtitles
On 12/11/2017 03:12, Carl Eugen Hoyos wrote: -put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration); +if (duration > 0) +put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration); In that case, the duration of the block is DefaultDuration (if it exists), completely different. Is it intended to say that a duration of 0 at this place means that the block has the default block duration if it exists? What is the use case for a duration of 0? (Note: the meaning of the a block duration of 0 is unclear to me, I'll ask on CELLAR mailing list to clarify that) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavcodec: fix field_order labelling
Le 29/08/2017 à 15:16, Tobias Rapp a écrit : On 27.08.2017 19:13, Marton Balint wrote: I agree that a lot of stuff in the codebase is consistent with the updated descriptions. However, as far as I see libavformat/mxfdec.c seems to follow the existing docs, so I think that needs changing as well. In mxfdec.c the value of field_order is constructed from VideoLineMap and FieldDominance. The VideoLineMap property indicates coded field order and the FieldDominance property indicated whether display field order is flipped compared to the coded order or not. So yes, mxfdec.c is following the current documentation of AVFieldOrder and applying the patch would conflict with the MXF specs, AFAICS. CC-ing Jérôme Martinez as he has much more experience with MXF real-world file variations. I think that the source of the issue is that different meanings were mapped to the same flag. My understanding is that we have 3 items (instead of 2) for interlaced content: - Store method (separate fields or interleaved fields) - Store order (Top or Bottom field first) - Display order (Top or Bottom field first) I understand that MOV (and MKV) have store method and store order, then display order is always store order. The patch would fix the issue between MOV/MKV specs and FFmpeg analysis. For MXF, looks like we have also display order which may be not store order, with "field dominance", from specs: "A value of 1 shall indicate that the first field is first in temporal order. A value of 2 shall indicate that the second field is the first in temporal order." Reading FFmpeg code, I understand something similar to Marton understanding and the patch can not be applied as is, there is a need to separate cases, but this implies that FFmpeg needs to handle 3 "flags" instead of 2, then there is a need to check all formats that the mapping is correct (e.g. I have no idea about the MJPEG stuff, looks like complex). I suggest a complex change, but I don't see another method: more AV_FIELD_*! AV_FIELD_UNKNOWN, AV_FIELD_PROGRESSIVE, AV_FIELD_STT: separate fields, top coded first, top displayed first AV_FIELD_SBB: separate fields, bottom coded first, bottom displayed first AV_FIELD_STB: separate fields, top coded first, bottom displayed first AV_FIELD_SBT: separate fields, bottom coded first, top displayed first AV_FIELD_ITT: interleaved fields, top coded first, top displayed first AV_FIELD_IBB: interleaved fields, bottom coded first, bottom displayed first AV_FIELD_ITB: interleaved fields, top coded first, bottom displayed first AV_FIELD_IBT: interleaved fields, bottom coded first, top displayed first For MXF, field_topness = (descriptor->video_line_map[0] + descriptor->video_line_map[1]) % 2 For MOV, it includes MKV (same spec) and is 'fiel' atom value AV_FIELD_STT: MOV 1, MXF frame_layout=SINGLE_FIELD field_topness=1 field_dominance=1 AV_FIELD_SBB: MOV 6, MXF frame_layout=SINGLE_FIELD field_topness=0 field_dominance=1 AV_FIELD_STB: MXF frame_layout=SINGLE_FIELD field_topness=1 field_dominance=0 AV_FIELD_SBT: MXF frame_layout=SINGLE_FIELD field_topness=0 field_dominance=0 AV_FIELD_ITT: MOV 9, MXF frame_layout=MIXED_FIELDS field_topness=1 field_dominance=1 AV_FIELD_IBB: MOV 14, MXF frame_layout=MIXED_FIELDS field_topness=0 field_dominance=1 AV_FIELD_STB: MXF frame_layout=MIXED_FIELDS field_topness=1 field_dominance=0 AV_FIELD_SBT: MXF frame_layout=MIXED_FIELDS field_topness=0 field_dominance=0 In other words, for MXF: - frame_layout is for 1st letter (new!) - video_line_map is for 2nd letter (was 1st letter) - field_dominance is for 3rd letter (was 2nd letter) and for MOV/MKV: - <8 or >=8 for 1st letter (current) - &0x7 for 2nd letter (current) - 3rd letter is a copy of 2nd letter (no "dominance" possible in MOV/MKV) Notes while reading MXF parsing in FFmpeg: MIXED_FIELDS: there is a "break", but IMO algo for field_order should apply SEGMENTED_FRAME: specs say "the value of field dominance has no meaning and should not be present." but if FieldDominance is in the file, FieldDominance is used qlso for SEGMENTED_FRAME (no "break"). height is also multiplied by 2 but specs say that height is already the frame height. But I have no file for testing. Note while reading e.g. FFV1 encoder code: I see: if (avctx->field_order == AV_FIELD_TT || avctx->field_order == AV_FIELD_TB) p->top_field_first = 1; if we have 8 cases, such kind of check maybe be huge (test on 4 values), maybe flags are more relevant rather that enums but it breaks more things I guess. Additionally I understand (but I am not a FFmpeg expert, confirmation?) that we lose info about which field should be displayed first (only which field is coded first is stored), and the decoder maps to top_field_first value only (no mapping to field_order). I wonder if we should not check display order instead of store order. Note for other formats: I don't know enough about other formats, e.g. v210 is interleaved
Re: [FFmpeg-devel] [PATCH] ffmpeg: fix setting field_order during muxing
Le 10/08/2017 à 04:43, James Almer a écrit : AVFrame.top_field_first doxy states "If the content is interlaced, is top field displayed first." And AVFieldOrder doxy defines: AV_FIELD_TB, //< Top coded first, bottom displayed first AV_FIELD_BT, //< Bottom coded first, top displayed first Fixes ticket #6577 IMHO the subject is complex, and everyone should be in sync with the purpose of each mux_par->field_order value and understand impact on other players (sometimes the MOV metadata is the only one available as the codec has no info e.g. uncompressed or jp2k) before changing current behavior in a so general manner. What I understood is that it is based on QuickTime specs (the mapping is 1 to 1 from fiel atom) and TN2162 and that they are not easy to understand and maybe misleading : the feedback I got in the last years about the meaning of this atom are not really in sync with specs having stored vs displayed, and such change may have an impact on how other players handle fiel atom or other metadata in the way it is currently implemented by FFmpeg. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/matroska: Support codec id V_FFV1 for FFV1.
Le 01/03/2017 à 15:51, wm4 a écrit : On Wed, 1 Mar 2017 15:00:27 +0100 Jerome Martinez <jer...@mediaarea.net> wrote: [...] then in 12 (or 24, or 36 months), the time that new FFmpeg versions propagate everywhere (we also open tickets for other demuxers in order to get the support of such files), the default is changed to V_FFV1. There are other demuxers than the FFmpeg one. Actually I suspect most trouble will be with ancient mkvmerge versions floating around and that everyone seems to want to use. I think the people loving ancient mkvmerge versions are not the ones using lossless formats, so it is not blocking from my point of view. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/matroska: Support codec id V_FFV1 for FFV1.
Le 01/03/2017 à 09:43, Carl Eugen Hoyos a écrit : 2017-03-01 4:40 GMT+01:00 Michael Niedermayer: On Tue, Feb 28, 2017 at 08:45:44PM +0100, Carl Eugen Hoyos wrote: Hi! Attached patch fixes ticket #6206 here. Please comment, Carl Eugen previous ffmpeg versions dont play these files What do you suggest? My suggestion: - support by the demuxer - for the muxer, support only if a specific explicit option is set, default stays AVI compatibility layer (no break in playback by old players). then in 12 (or 24, or 36 months), the time that new FFmpeg versions propagate everywhere (we also open tickets for other demuxers in order to get the support of such files), the default is changed to V_FFV1. Sorry I don't know enough FFmpeg for doing the corresponding patch myself. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 00/11] CRC32 support for Matroska muxer
Le 06/10/2016 à 12:29, Michael Niedermayer a écrit : Does this reduce writing speed ? in the same manner as e.g. reducing FFV1 writing speed with default configuration, i.e. with CRC per slice (same kind of job). On my machine (i7 from 2012), CRC computing takes less than 1% of the CPU when I decode FFV1, so I estimate the impact of CRC on both FFV1 level and Matroska level to less than 2% in case FFV1 is used. I don't think it would be a more with e.g. AVC as frame byte size is smaller. for example when remuxing high bitrate data like rawvideo ? On my machine (i7 from 2012), with the "4 bytes at once" algorithm used by FFmpeg if I well understood FFmpeg code on this part, I benched CRC computing at 1 GB/s per core, so it should be more than enough also for rawvideo. CRC element could be an option as it is for FFV1, but I argue for having it set by default, for the same reasons it is set by default with FFV1. [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/ffv1dec: fix some unsupported pix_fmt
Le 13/06/2016 à 19:31, Jerome Martinez a écrit : [...] - JPEG 2000 RCT 9/10/12/14 bit depths with alpha are mapped to a FFmpeg pix_fmt without alpha (e.g. AV_PIX_FMT_GBRP9 for 9-bit with alpha), which is not expected. Sorry, I forgot a remark about this part of the patch: the decoder can decode the bitstream, the issue is that the alpha content is trashed: if (lbd) *((uint32_t*)(src[0] + x*4 + stride[0]*y)) = b + (g<<8) + (r<<16) + (a<<24); else { *((uint16_t*)(src[0] + x*2 + stride[0]*y)) = b; *((uint16_t*)(src[1] + x*2 + stride[1]*y)) = g; *((uint16_t*)(src[2] + x*2 + stride[2]*y)) = r; } (lbd is 1 if s->avctx->bits_per_raw_sample <= 8, "a" is used in that case) In my patch, I chose to forbid the decoding in order to not let the user think that the decoding is lossless, but another possibility is to add a warning. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/ffv1dec: fix some unsupported pix_fmt
Le 13/06/2016 à 19:55, Michael Niedermayer a écrit : On Mon, Jun 13, 2016 at 07:31:15PM +0200, Jerome Martinez wrote: FFV1 decoder: When checking pix_fmt mapping, some bitstreams are mapped to an incorrect pix_fmt instead of being rejected (ENOSYS). Actually, such bitstreams are not supported (FFmpeg encoder does not produce such bitstream, such bitstream may come only from another encoder for the moment). - JPEG 2000 RCT 11/13/15/16 bit depths are mapped to a 8-bit FFmpeg pix_fmt (e.g. bgr0), which is not expected. - JPEG 2000 RCT 9/10/12/14 bit depths with alpha are mapped to a FFmpeg pix_fmt without alpha (e.g. AV_PIX_FMT_GBRP9 for 9-bit with alpha), which is not expected. The order for choosing the pix_fmt is changed to the one used by YCbCr selection (<=8 bit first). " && !f->transparency" is added to the other lines. ffv1dec.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) 27efc1a9821576a2a5bd8d1feaaf6c584fc69a62 0001-avcodec-ffv1dec-fix-some-unsupported-pix_fmt.patch From 90bfd748b0e25d7a0be037280f4a0a40242f8d27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jer...@mediaarea.net> Date: Mon, 13 Jun 2016 19:18:22 +0200 Subject: [PATCH] avcodec/ffv1dec: fix some unsupported pix_fmt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jérôme Martinez <jer...@mediaarea.net> --- libavcodec/ffv1dec.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index d2bf3a8..6a932b2 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -768,17 +768,18 @@ static int read_header(FFV1Context *f) "chroma subsampling not supported in this colorspace\n"); return AVERROR(ENOSYS); } -if ( f->avctx->bits_per_raw_sample == 9) +if ( f->avctx->bits_per_raw_sample <= 8 && !f->transparency) +f->avctx->pix_fmt = AV_PIX_FMT_0RGB32; +else if (f->avctx->bits_per_raw_sample <= 8 && f->transparency) +f->avctx->pix_fmt = AV_PIX_FMT_RGB32; +else if (f->avctx->bits_per_raw_sample == 9 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP9; -else if (f->avctx->bits_per_raw_sample == 10) +else if (f->avctx->bits_per_raw_sample == 10 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP10; -else if (f->avctx->bits_per_raw_sample == 12) +else if (f->avctx->bits_per_raw_sample == 12 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP12; -else if (f->avctx->bits_per_raw_sample == 14) +else if (f->avctx->bits_per_raw_sample == 14 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP14; -else -if (f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_RGB32; -else f->avctx->pix_fmt = AV_PIX_FMT_0RGB32; an else "error" feels missing I did exactly as the code few lines above: if f->colorspace == 0 and f->avctx->bits_per_raw_sample >=17, there is also no else: } else if (f->avctx->bits_per_raw_sample == 16 && f->transparency){ switch(16 * f->chroma_h_shift + f->chroma_v_shift) { case 0x00: f->avctx->pix_fmt = AV_PIX_FMT_YUVA444P16; break; case 0x10: f->avctx->pix_fmt = AV_PIX_FMT_YUVA422P16; break; case 0x11: f->avctx->pix_fmt = AV_PIX_FMT_YUVA420P16; break; } } // an else "error" feels missing here too. pix_fmt would not be initialized after the patch When I test the code, pix_fmt is initialized to -1 (AV_PIX_FMT_NONE) before running this part, and the error is caught by the "format not supported" message, which looks like a good message for that. I have no problem with adding such else in the patch, but the code would not be coherent (an else "error" feels missing if f->colorspace == 0, and else "error" does not feel missing if f->colorspace == 1), so I would like to have the confirmation that this is what you really want. Note: BTW about missing things, there is no av_log() message when f->colorspace == 0 && f->transparency && !f->chroma_plane && ->avctx->bits_per_raw_sample >8 (=gray+alpha with a bit depth of more than 8), just a "else return AVERROR(ENOSYS);", I was actually willing to remove these 2 lines in order to have more coherency (no "else" so "format not supported" message a bit later in the code) but I did not want to add different fixes in the same patch. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/ffv1dec: fix some unsupported pix_fmt
FFV1 decoder: When checking pix_fmt mapping, some bitstreams are mapped to an incorrect pix_fmt instead of being rejected (ENOSYS). Actually, such bitstreams are not supported (FFmpeg encoder does not produce such bitstream, such bitstream may come only from another encoder for the moment). - JPEG 2000 RCT 11/13/15/16 bit depths are mapped to a 8-bit FFmpeg pix_fmt (e.g. bgr0), which is not expected. - JPEG 2000 RCT 9/10/12/14 bit depths with alpha are mapped to a FFmpeg pix_fmt without alpha (e.g. AV_PIX_FMT_GBRP9 for 9-bit with alpha), which is not expected. The order for choosing the pix_fmt is changed to the one used by YCbCr selection (<=8 bit first). " && !f->transparency" is added to the other lines. From 90bfd748b0e25d7a0be037280f4a0a40242f8d27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?=Date: Mon, 13 Jun 2016 19:18:22 +0200 Subject: [PATCH] avcodec/ffv1dec: fix some unsupported pix_fmt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jérôme Martinez --- libavcodec/ffv1dec.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index d2bf3a8..6a932b2 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -768,17 +768,18 @@ static int read_header(FFV1Context *f) "chroma subsampling not supported in this colorspace\n"); return AVERROR(ENOSYS); } -if ( f->avctx->bits_per_raw_sample == 9) +if ( f->avctx->bits_per_raw_sample <= 8 && !f->transparency) +f->avctx->pix_fmt = AV_PIX_FMT_0RGB32; +else if (f->avctx->bits_per_raw_sample <= 8 && f->transparency) +f->avctx->pix_fmt = AV_PIX_FMT_RGB32; +else if (f->avctx->bits_per_raw_sample == 9 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP9; -else if (f->avctx->bits_per_raw_sample == 10) +else if (f->avctx->bits_per_raw_sample == 10 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP10; -else if (f->avctx->bits_per_raw_sample == 12) +else if (f->avctx->bits_per_raw_sample == 12 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP12; -else if (f->avctx->bits_per_raw_sample == 14) +else if (f->avctx->bits_per_raw_sample == 14 && !f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_GBRP14; -else -if (f->transparency) f->avctx->pix_fmt = AV_PIX_FMT_RGB32; -else f->avctx->pix_fmt = AV_PIX_FMT_0RGB32; } else { av_log(f->avctx, AV_LOG_ERROR, "colorspace not supported\n"); return AVERROR(ENOSYS); -- 2.7.0.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] Remote participation options for IETF session on MKV/FFV1 at July 22 @ 9 CEST
Le 21/07/2015 20:03, Ronald S. Bultje a écrit : +1. I can't stress how important this is. In addition, the spec should be the master, not any one implementation (because then the bugs in that one implementation will be the spec, regardless of what the bug is). In theory, spec should be the reference, I totally agree. In practice, both Matroska and FFV1 formal specifications show up after these formats are widely used, without relying on any specification. So saying that the specification is the reference does not make a lot of sense here. I argue for: - previous and current versions: specifications are more an official state of the art and we try to have a specification the most compatible with most used tools. If 2 tools are incompatible, we discuss with developers case by case about the best method for fixing the incompatibility and we write the decision as part of the specification. Example of specification which takes care of compatibility with files in the wild: O is a reserved value. Encoders MUST NOT store bits_per_raw_sample = 0. Decoders SHOULD accept and interpret bits_per_raw_sample = 0 as 8. - next version: the spec is the master, not any one implementation, and we have 2-3 independent implementations ready *before* the formal release of the specification. FYI, we are on the way of having a completely independent FFV1 parser/decoder in libmediainfo and a complete Matroska and FFV1 conformance checker tool, without relying on other libraries (e.g. ffmpeg, libav, libmatroska) so we hope to catch any issue in the specs and/or files created by other tools before the formal release of specifications. Please comment. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Merge of FrameHeader01() and GlobalHeader()
Le 20/05/2015 03:44, Michael Niedermayer a écrit : [...] If you want to allow multiple w/h/colorspace, its probably better to allow multiple global headers as in h264/h265 and put a index in the frame header to choose one of the parameter sets some of the tables are also large IIRC initial_state_delta is. it makes not much sense for this to be anywhere but global Proposals for the next version of FFV1 is planned ;-). But for the moment, I focus on having a specification of the existing versions. maybe none of these headers should have been called frame header i dont know ... I thought it would have been better to separates subjects (on one side the merge, on another side the renaming). Anyway, new patch attached: - FrameHeader() renamed to Parameters() - additional line in initial_state_delta[ i ][ j ][ k ] description removed. https://github.com/MediaArea/FFV1/tree/HeaderMerge is also up to date. LyX Document From 5580818c24c5de2a721d4bce505a6cfb9f9833f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Wed, 20 May 2015 09:11:27 +0200 Subject: [PATCH] Merge of FrameHeader01() and GlobalHeader() FrameHeader01() and GlobalHeader() have a lot of common fields and having a common function + default value for fields unused in previous versions is less complex and more coherent than repeating the common part. They are merged and renamed to Parameters(). --- ffv1.lyx | 528 --- 1 file changed, 201 insertions(+), 327 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index 01f7308..d50eba7 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -1860,7 +1860,7 @@ alternative state transition table The alternative state transition table has been build using iterative minimizati on of frame sizes and generally performs better than the default. To use it, the coder_type has to be set to 2 and the difference to the - default has to be stored in the header. + default has to be stored in the parameters. The reference implemenation of FFV1 in FFmpeg uses this table by default at the time of this writing when Range coding is used. \end_layout @@ -2557,6 +2557,10 @@ In the case of a bitstream with version = 2, a configuration record is \begin_inset Newline newline \end_inset +It contains the parameters used for all frames. +\begin_inset Newline newline +\end_inset + The size of the configuration record, NumBytes, is supplied by the underlying container. \end_layout @@ -2642,7 +2646,7 @@ ConfigurationRecordIsPresent = 1 \begin_inset space ~ \end_inset -GlobalHeader( ) +Parameters( ) \end_layout \end_inset @@ -3172,7 +3176,7 @@ if( keyframe !ConfigurationRecordIsPresent) \begin_inset space ~ \end_inset -FrameHeader01( ) +Parameters( ) \end_layout \end_inset @@ -5155,16 +5159,12 @@ reserved for future use \end_layout \begin_layout Subsection -Header -\end_layout - -\begin_layout Subsubsection -Version 0 and 1 +Parameters \end_layout \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=15 columns=2 +lyxtabular version=3 rows=34 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -5173,7 +5173,7 @@ Version 0 and 1 \begin_inset Text \begin_layout Plain Layout -FrameHeader01( ) { +Parameters( ) { \end_layout \end_inset @@ -5224,6 +5224,92 @@ ur /cell /row row +cell alignment=center valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +if( version 2 ) +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout + +\end_layout + +\end_inset +/cell +/row +row +cell alignment=center valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +micro_version +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +ur +\end_layout + +\end_inset +/cell +/row +row cell alignment=left valignment=top topline=true leftline=true usebox=none \begin_inset Text @@ -5482,7 +5568,7 @@ if( version 0 ) /cell /row row -cell alignment=center valignment=top topline=true leftline=true usebox=none +cell alignment=left valignment=top topline=true leftline=true usebox=none \begin_inset Text \begin_layout Plain
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Merge of FrameHeader01() and GlobalHeader()
Le 19/05/2015 21:15, Michael Niedermayer a écrit : On Mon, May 18, 2015 at 09:04:01PM +0200, Jerome Martinez wrote: FrameHeader01() and GlobalHeader() have a lot of common fields and having a common function + default value for fields unused in previous versions is less complex and more coherent than repeating the common part. maybe but calling the GlobalHeader FrameHeader is very confusing and wrong From my point of view, the GlobalHeader() is still a frame header because it still contains the same pieces of information about frames (frame width, frame height, frame bit depth...), and it is just not repeated (moved from the beginning of the frame to the configuration record). During the decoding, it is exactly like if the old GlobalHeader() is repeated per keyframe (a global header is equivalent to a frame header repeated per keyframe), and actually the exact same FFV1Context members are used in ffv1dec.c whatever is the version of the FFV1 bitstream. Actually, and still from my point of view, FrameHeader() is very confusing and wrong as much as FrameHeader01() in the original specification of v0/1 (e.g. if we have a stream with one keyframe and all other frames not keyframes, we have only one FrameHeader01() call for all frames, as with GlobalHeader(), so FrameHeader01() is confusing in that case) so I don't add more confusion or more wrongness. I also tried not to change everything, and I just upgraded the original FrameHeader01() function without renaming it (I only removed the versioning). I also added the following sentence in the Configuration Record subsection: LyX Document It (the Configuration Record) contains the frame header used for all frames. Isn't it explicit enough? And I don't find a better wording for something which is per frame (actually not per frame, only per keyframes) in v0/1 and global in v2+. Header()? (I am not a big fan of it because it is too much general) Parameters()? I argue for not renaming too much functions before reaching a larger audience (IETF and so on) and getting their points of view. [...] @@ -7939,6 +7809,10 @@ pred = j ? initial_states[ i ][j - 1][ k ] : 128 initial_state[ i ][ j ][ k ] = ( pred + initial_state_delta[ i ][ j ][ k ] ) 255 +\begin_inset Newline newline +\end_inset + +Inferred to be 0 if not present. what is inferred to be 0? initial_state? initial_state_delta? i think this could be misunderstood as the paragraph is talking about multiple variables I planned to reword the description of initial_state_delta (separation between the bitstream syntax which should be in this subsection and formulas which should be in another section dedicated to initial_state description) in another patch, but in the meanwhile this could be misunderstood, true. I propose to change to Theses formulas are not applied if initial_state_delta[ i ][ j ][ k ] is not present or Theses formulas are not applied if LyX Document states_coded is 0 or just remove the line. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] FFV1 specification: Merge of FrameHeader01() and GlobalHeader()
From c6f16e561d40972e058f4e163ff753bce8fc8acc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Mon, 18 May 2015 20:59:09 +0200 Subject: [PATCH] Merge of FrameHeader01() and GlobalHeader() FrameHeader01() and GlobalHeader() have a lot of common fields and having a common function + default value for fields unused in previous versions is less complex and more coherent than repeating the common part. --- ffv1.lyx | 530 --- 1 file changed, 204 insertions(+), 326 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index 01f7308..e38389a 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -2557,6 +2557,10 @@ In the case of a bitstream with version = 2, a configuration record is \begin_inset Newline newline \end_inset +It contains the frame header used for all frames. +\begin_inset Newline newline +\end_inset + The size of the configuration record, NumBytes, is supplied by the underlying container. \end_layout @@ -2642,7 +2646,7 @@ ConfigurationRecordIsPresent = 1 \begin_inset space ~ \end_inset -GlobalHeader( ) +FrameHeader( ) \end_layout \end_inset @@ -3172,7 +3176,7 @@ if( keyframe !ConfigurationRecordIsPresent) \begin_inset space ~ \end_inset -FrameHeader01( ) +FrameHeader( ) \end_layout \end_inset @@ -5155,16 +5159,12 @@ reserved for future use \end_layout \begin_layout Subsection -Header -\end_layout - -\begin_layout Subsubsection -Version 0 and 1 +Frame Header \end_layout \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=15 columns=2 +lyxtabular version=3 rows=34 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -5173,7 +5173,7 @@ Version 0 and 1 \begin_inset Text \begin_layout Plain Layout -FrameHeader01( ) { +FrameHeader( ) { \end_layout \end_inset @@ -5224,6 +5224,92 @@ ur /cell /row row +cell alignment=center valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +if( version 2 ) +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout + +\end_layout + +\end_inset +/cell +/row +row +cell alignment=center valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +micro_version +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +ur +\end_layout + +\end_inset +/cell +/row +row cell alignment=left valignment=top topline=true leftline=true usebox=none \begin_inset Text @@ -5482,7 +5568,7 @@ if( version 0 ) /cell /row row -cell alignment=center valignment=top topline=true leftline=true usebox=none +cell alignment=left valignment=top topline=true leftline=true usebox=none \begin_inset Text \begin_layout Plain Layout @@ -5692,7 +5778,7 @@ br \begin_inset space ~ \end_inset -QuantizationTable( 0 ) +if ( version 1 ) { \end_layout \end_inset @@ -5708,52 +5794,42 @@ QuantizationTable( 0 ) /cell /row row -cell alignment=center valignment=top topline=true bottomline=true leftline=true usebox=none +cell alignment=center valignment=top topline=true leftline=true usebox=none \begin_inset Text \begin_layout Plain Layout -} -\end_layout +\begin_inset space ~ +\end_inset + +\begin_inset space ~ \end_inset -/cell -cell alignment=center valignment=top topline=true bottomline=true leftline=true rightline=true usebox=none -\begin_inset Text -\begin_layout Plain Layout -\end_layout +\begin_inset space ~ +\end_inset + +\begin_inset space ~ \end_inset -/cell -/row -/lyxtabular + +\begin_inset space ~ \end_inset -\end_layout +\begin_inset space ~ +\end_inset -\begin_layout Subsubsection -Version 3 -\end_layout -\begin_layout Standard -Version 2 and later files use a global header and a per frame header. -\end_layout +\begin_inset space ~ +\end_inset -\begin_layout Standard -\begin_inset Tabular -lyxtabular version=3 rows=28 columns=2 -features rotate=0 tabularvalignment=middle -column alignment=left valignment=top -column alignment=center valignment=top -row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text -\begin_layout Plain Layout -GlobalHeader( ) { +\begin_inset space ~ +\end_inset + +num_h_slices - 1
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Add more details about the configuration record
Le 14/05/2015 18:57, Michael Niedermayer a écrit : [...] +reserved the reserved bit is not defined, This way one could misunderstand it as if it was allowed to add such bits in the encoder I don't see it as something allowed (i.e. as a user data for private use) but you're right, it is not defined and it should be. Renamed asreserved_for_future_useLyX Document and added LyX Document reserved_for_future_use has semantics that are reserved for future use. Encoders conforming to this version of this specification SHALL NOT write this value. Decoders conforming to this version of this specification SHALL ignore its value. From ebe963b70474e2f850223d93cce97d82c3b95990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Fri, 15 May 2015 12:43:36 +0200 Subject: [PATCH] Add more details about the configuration record Version 2+ of the format has a configuration record which is in the underlying container track definition. GlobalHeader definition is split in 2 parts: - a configuration record part, including more details about how to find it in AVI and MP4, including the algorithm for how to skip remaining bits directly in the bitstream definition, including the crc_parity, - the header part itself, which is the prveious GlobalHeader definition minus crc_parity. crc_parity definition is also split and moved in the right subsections: - configuration_record_crc_parity for the configuration record part subsection, - slice_crc_parity for the slice subsection. --- ffv1.lyx | 583 --- 1 file changed, 521 insertions(+), 62 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index c8bb2b7..01f7308 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -893,6 +893,20 @@ Range a...b means any value starting from a to b, inclusive. \end_layout +\begin_layout Subsection +Bitstream functions +\end_layout + +\begin_layout Description +remaing_bits_in_bitstream( +\begin_inset space ~ +\end_inset + +) means the count of remaining bits after the current position in the bitstream. + It is computed from the NumBytes value multiplied by 8 minus the count + of bits already read by the bitstream parser. +\end_layout + \begin_layout Section General Description \end_layout @@ -2525,31 +2539,49 @@ The same context which is initialized to 128 is used for all fields in the header. \end_layout +\begin_layout Standard +Default values at the decoder initialization phase: +\end_layout + +\begin_layout Description +ConfigurationRecordIsPresent is set to 0. +\end_layout + \begin_layout Subsection -Frame +Configuration Record +\end_layout + +\begin_layout Standard +In the case of a bitstream with version = 2, a configuration record is + stored in the the underlying container, at the track header level. +\begin_inset Newline newline +\end_inset + +The size of the configuration record, NumBytes, is supplied by the underlying + container. \end_layout \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=9 columns=2 +lyxtabular version=3 rows=7 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top row -cell alignment=left valignment=top topline=true leftline=true usebox=none +cell alignment=left valignment=top topline=true bottomline=true leftline=true usebox=none \begin_inset Text \begin_layout Plain Layout -Frame( ) { +ConfigurationRecord( NumBytes ) { \end_layout \end_inset /cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +cell alignment=center valignment=top topline=true bottomline=true leftline=true rightline=true usebox=none \begin_inset Text \begin_layout Plain Layout -type + \end_layout \end_inset @@ -2575,7 +2607,7 @@ type \begin_inset space ~ \end_inset -keyframe +ConfigurationRecordIsPresent = 1 \end_layout \end_inset @@ -2584,7 +2616,7 @@ keyframe \begin_inset Text \begin_layout Plain Layout -br + \end_layout \end_inset @@ -2610,7 +2642,42 @@ br \begin_inset space ~ \end_inset -if( keyframe ) { +GlobalHeader( ) +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout + +\end_layout + +\end_inset +/cell +/row +row +cell alignment=left valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +while( remaing_bits_in_bitstream( ) 32) \end_layout \end_inset @@ -2661,7 +2728,7 @@ if( keyframe ) { \begin_inset space ~ \end_inset -if( version 2 ) +reserved_for_future_use \end_layout \end_inset @@ -2670,14 +2737,14 @@ if( version 2 ) \begin_inset Text \begin_layout Plain Layout - +u(1) \end_layout \end_inset
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Add more details about the configuration record
Le 14/05/2015 00:48, Michael Niedermayer a écrit : nut and ffm surely work too I have doubts there are lot of FFV1 content in such container format ;-) Anyway: - Patch updated with NUT file format information - I tested FFmpeg for the support of FFV1 in FFM (source MOV files are there https://github.com/MediaArea/MediaConch/tree/master/SampleTestFiles/FFV1 ) ffmpeg -i ffv1_0.mov -vcodec copy ffv1_0.ffm ffmpeg -i ffv1_0.ffm -f null /dev/null is OK. ffmpeg -i ffv1_3.mov -vcodec copy ffv1_3.ffm ffmpeg -i ffv1_3.ffm -f null /dev/null is NOK [ffv1 @ 004F2600] read_quant_table error [ffv1 @ 00515AC0] Cannot decode non-keyframe without valid keyframe [ffv1 @ 005296A0] Cannot decode non-keyframe without valid keyframe [ffv1 @ 00537BC0] Cannot decode non-keyframe without valid keyframe [ffv1 @ 004D22A0] Cannot decode non-keyframe without valid keyframe [ffv1 @ 004D2700] Cannot decode non-keyframe without valid keyframe [ffv1 @ 004D3300] Cannot decode non-keyframe without valid keyframe Error while decoding stream #0:0: Invalid data found when processing input I looked for the GlobalHeader presence in ffv1_3.ffm (FFM2 format) but it is not present (I don't find the 4 bytes of CRC in the file) Looks like FFmpeg creates invalid FFV1 version 3 in NUT files (frames are OK but the GlobalHeader part is missing) and is not able to read such file. I don't find any documentation about FFM format specification (I just find some reference in https://www.ffmpeg.org/ffserver.html#FFM_002c-FFM2-formats ) but it looks like FFM supports extra_data (AVFMT_GLOBALHEADER, CODEC_FLAG_GLOBAL_HEADER, in avformat/ffm_*.c) and the issue is maybe in ffv1enc/ffv1dec: I see in ffmenc.c: if (codec-flags CODEC_FLAG_GLOBAL_HEADER) { avio_wb32(pb, codec-extradata_size); avio_write(pb, codec-extradata, codec-extradata_size); } but ffv1enc.c does not contain any reference to CODEC_FLAG_GLOBAL_HEADER as libx264.c / libx265.c / libfaac.c / ... do. If don't now enough FFmpeg source code for going further in my inspection: it is a headache because FFM2 format looks like to rely on CODEC_FLAG_GLOBAL_HEADER (which is not part of FFM2, it is in FFMpeg source code) for writing extradata_size in the container, but extradata may be present or not present depending of the FFV1 version, and I don't see how to write that in the code: if I force ffv1 for writing extradata_size in FFM file in all cases, I break previous FFV1 version 0-1 files, and if I let as is I can not write extradata_size in FFM file. Someone for helping? It should make it clear that these are not the only containers supported but that nearly any container can be used Agreed. Is it OK with: This configuration record can be placed in any file format supporting configuration records, fitting as much as possible with how the file format uses to store configuration records. The configuration record storage place and NumBytes are currently defined and supported by this specification for the following container formats: ok Patch updated. From 4c71831de018cfa5279610cace0183e8db80dcee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Thu, 14 May 2015 10:59:48 +0200 Subject: [PATCH] Add more details about the configuration record Version 2+ of the format has a configuration record which is in the underlying container track definition. GlobalHeader definition is split in 2 parts: - a configuration record part, including more details about how to find it in AVI and MP4, including the algorithm for how to skip remaining bits directly in the bitstream definition, including the crc_parity, - the header part itself, which is the prveious GlobalHeader definition minus crc_parity. crc_parity definition is also split and moved in the right subsections: - configuration_record_crc_parity for the configuration record part subsection, - slice_crc_parity for the slice subsection. --- ffv1.lyx | 565 --- 1 file changed, 503 insertions(+), 62 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index c8bb2b7..41f2015 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -893,6 +893,20 @@ Range a...b means any value starting from a to b, inclusive. \end_layout +\begin_layout Subsection +Bitstream functions +\end_layout + +\begin_layout Description +remaing_bits_in_bitstream( +\begin_inset space ~ +\end_inset + +) means the count of remaining bits after the current position in the bitstream. + It is computed from the NumBytes value multiplied by 8 minus the count + of bits already read by the bitstream parser. +\end_layout + \begin_layout Section General Description \end_layout @@ -2525,31 +2539,49 @@ The same context which is initialized to 128 is used for all fields in the header. \end_layout +\begin_layout Standard +Default values at the decoder initialization phase: +\end_layout + +\begin_layout Description
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Add more details about the configuration record
Le 13/05/2015 21:58, Michael Niedermayer a écrit : Does the text somewhere say why just avi and mp4 are listed as containers ? (i didnt spot that but i might have missed it) They are the only containers I know supporting FFV1 (Matroska is not listed here because it does not support FFV1 directly: it uses the AVI compatibility layer, so currently implementation in Matroska is defined by implementation in AVI + definition of AVI compatibility layer in Matroska) It is not possible to be exhaustive, there is no standardized way to say that if there is a configuration record, it must be at here, and e.g. for MOV the glbl box is never defined in Apple specs not in ISO specs, it is specific to FFmpeg even if it is aimed to be used for all formats requesting a configuration record. If by chance ISO accepts FFV1 in MP4, they could request that the configuration record is in a fv1C box or other change instead of the glbl box... Usually the file format maintainer writes rules but if I understood well what happened in the case of FFV1 in MP4/MOV, FFmpeg decided to use home made glbl box and I try to write such reality in the specification, not easy. So this is case per case, file format per file format, I list what I am aware of. Are you aware of another possible container for FFV1 and supported by FFmpeg? It should make it clear that these are not the only containers supported but that nearly any container can be used Agreed. Is it OK with: This configuration record can be placed in any file format supporting configuration records, fitting as much as possible with how the file format uses to store configuration records. The configuration record storage place and NumBytes are currently defined and supported by this specification for the following container formats: Note: when I finish to technically update the specification, I'll ask a native English speaker for reviewing the whole spec. LyX Document [...] @@ -2728,7 +3056,7 @@ if( version 2 ) \begin_inset space ~ \end_inset -FrameHeader01( ) +if( keyframe !ConfigurationRecordIsPresent) is it better to add indirection here instead of spelling out that its version 2 ? at this moment of the parsing, version is not defined in the case of a bitstream conforming to version 2 (it is defined just after). In the current FFV1 specification, it does not make sense (version is tested before being defined) I don't see how to describe correctly the bitstream with version 2 here. this is not an indirection for nothing, this is avoiding to use a single field for different purpose: version is for indicating the version, not for knowing if there was a configuration record. this is the reason I propose to have 2 different fields: 1 for version, 1 for knowing if there was a configuration record before the parsing of a frame. This does not prevent the decoder to use a different algorithm, but a specification is not optimized code: saying that version is 2 before we read version from the bitstream is not correct from my point of view (we don't know the version because we did not read it, so we can not say that version is 2). From my point of view, this is also more future proof: you may decide that version 5 can accept both cases (with or without configuration record), it would be easier to update the specification (with version 2, you forbids forever any new version without configuration records). ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] FFV1 specification: Add more details about the configuration record
Add more details about the configuration record Version 2+ of the format has a configuration record which is in the underlying container track definition. GlobalHeader definition is split in 2 parts: - a configuration record part, including more details about how to find it in AVI and MP4, including the algorithm for how to skip remaining bits directly in the bitstream definition, including the crc_parity, - the header part itself, which is the prveious GlobalHeader definition minus crc_parity. crc_parity definition is also split and moved in the right subsections: - configuration_record_crc_parity for the configuration record part subsection, - slice_crc_parity for the slice subsection. From 1f83b63f2554d765f98b934b199734b5c82fe721 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Wed, 13 May 2015 18:43:49 +0200 Subject: [PATCH] Add more details about the configuration record Version 2+ of the format has a configuration record which is in the underlying container track definition. GlobalHeader definition is split in 2 parts: - a configuration record part, including more details about how to find it in AVI and MP4, including the algorithm for how to skip remaining bits directly in the bitstream definition, including the crc_parity, - the header part itself, which is the prveious GlobalHeader definition minus crc_parity. crc_parity definition is also split and moved in the right subsections: - configuration_record_crc_parity for the configuration record part subsection, - slice_crc_parity for the slice subsection. --- ffv1.lyx | 499 +++ 1 file changed, 437 insertions(+), 62 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index c8bb2b7..9894a0f 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -893,6 +893,20 @@ Range a...b means any value starting from a to b, inclusive. \end_layout +\begin_layout Subsection +Bitstream functions +\end_layout + +\begin_layout Description +remaing_bits_in_bitstream( +\begin_inset space ~ +\end_inset + +) means the count of remaining bits after the current position in the bitstream. + It is computed from the NumBytes value multiplied by 8 minus the count + of bits already read by the bitstream parser. +\end_layout + \begin_layout Section General Description \end_layout @@ -2525,31 +2539,49 @@ The same context which is initialized to 128 is used for all fields in the header. \end_layout +\begin_layout Standard +Default values at the decoder initialization phase: +\end_layout + +\begin_layout Description +ConfigurationRecordIsPresent is set to 0. +\end_layout + \begin_layout Subsection -Frame +Configuration Record +\end_layout + +\begin_layout Standard +In the case of a bitstream with version = 2, a configuration record is + stored in the the underlying container, at the track header level. +\begin_inset Newline newline +\end_inset + +The size of the configuration record, NumBytes, is supplied by the underlying + container. \end_layout \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=9 columns=2 +lyxtabular version=3 rows=7 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top row -cell alignment=left valignment=top topline=true leftline=true usebox=none +cell alignment=left valignment=top topline=true bottomline=true leftline=true usebox=none \begin_inset Text \begin_layout Plain Layout -Frame( ) { +ConfigurationRecord( NumBytes ) { \end_layout \end_inset /cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +cell alignment=center valignment=top topline=true bottomline=true leftline=true rightline=true usebox=none \begin_inset Text \begin_layout Plain Layout -type + \end_layout \end_inset @@ -2575,7 +2607,7 @@ type \begin_inset space ~ \end_inset -keyframe +ConfigurationRecordIsPresent = 1 \end_layout \end_inset @@ -2584,7 +2616,7 @@ keyframe \begin_inset Text \begin_layout Plain Layout -br + \end_layout \end_inset @@ -2610,7 +2642,42 @@ br \begin_inset space ~ \end_inset -if( keyframe ) { +GlobalHeader( ) +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout + +\end_layout + +\end_inset +/cell +/row +row +cell alignment=left valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +while( remaing_bits_in_bitstream( ) 32) \end_layout \end_inset @@ -2661,7 +2728,7 @@ if( keyframe ) { \begin_inset space ~ \end_inset -if( version 2 ) +reserved \end_layout \end_inset @@ -2670,14 +2737,14 @@ if( version 2 ) \begin_inset Text \begin_layout Plain
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Reduce redundancy in the description of xxPlane() and xxLine()
Le 03/05/2015 04:57, Michael Niedermayer a écrit : i dont think its a good idea to replace a named variable in a for () statement by a construct so long that it needs a linebreak in the text output [...] I hesitated and this is a very good point, so I replaced by 2 named variables: - plane_count which is the count of planes - quant_table_index_count which is the count of quant_table indexes Note that plane_count in the ffv1 source code is actually the count of quant_table indexes, and is _not_ the actual count of planes, I think it is dangerous to have this name for the count of quant_table indexes and I'll propose a ffv1 source code patch for renaming it. also a more critical problem is that this patch makes the spec less well defined. It does not. As written in the patch description: the order of planes is already defined in the General section and has no impact on the bitstream. It is referencing the following sentences in the General section: In the case of the normal YCbCr colorspace the Y plane is coded first followed by the Cb and Cr planes, if an Alpha/transparency plane exists, it is coded last. In the case of the JPEG2000-RCT colorspace the lines are interleaved to improve caching efficiency since it is most likely that the RCT will immediately be converted to RGB during decoding; the interleaved coding order is also Y,Cb,Cr,Alpha. It is not good to have redundancy in a specification. Additionally, this is a limitation in the bitstream syntax which should not be present from my point of view, because the bitstream does not care about the type of the plane: Y, color or alpha, the bitstream syntax is same. Plane() and Line() are generic. so the description of planes should not be in the bitstream syntax. I don't see how I would define LumaPlane, CbPlane, CrPlane, AlphaPlane without repeating 99% of the description in each function. And it would limit future extension: it would be so easy to add any other color space e.g. XYZ (used by Cinema, see DCP packages), RGB, sRGB, CMYK, HSV or HSL, why not creating a generic bitstream syntax instead of forcing YUV? Actually, it is not the future (see below). Theres no ambiguity in what a Luma, Cb, Cr, Alpha something is but a plane 0 plane 1 plane 2, line 0 line 1 line 2 line 3 which is which ? Actually, if I well understood, there is already an issue with the way you describe the bitstream. In the case of version = 4, colorspace = 1 and slice_coding_mode = 1, you describe lines this way: LumaLine[y] CbLine[y] CrLine[y] but from the source code, I understand that: BLine[y] GLine[y] RLine[y] if we keep xxLine, code should be transformed from: LumaLine[y] CbLine[y] CrLine[y] to if (slice_coding_mode == 0) { LumaLine[y] CbLine[y] CrLine[y] } if (slice_coding_mode == 1) { BLine[y] GLine[y] RLine[y] } so from my point of view your description (in the General section and in the bitstream syntax) is already wrong (as well is the definition of chroma_planes which should be transformed to must be 1 in the case of colorspace = 1). So this patch fixes one issue (no LumaLine / CbLine / CrLine in the case of version = 4, colorspace = 1 and slice_coding_mode = 1), and I plan to fix the other issues (wrong description of planes and wrong description of chroma_planes in the General section and in in the case of version = 4, colorspace = 1 and slice_coding_mode = 1) in a future patch. This is leading to a proposal of bitstream change I would like to propose for version 4, but it is maybe better to discuss of this proposal in its own thread (and it is not the purpose of this patch). From ba2af678d50c2c3fa089eb9cee5b73da92bc2a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Sun, 3 May 2015 11:16:30 +0200 Subject: [PATCH] Reduce redundancy in the description of xxPlane() and xxLine(). LumaPlane(), CbPlane(), CrPlane() and AlphaPlane() are actually same; the order of planes is already defined in the General section and has no impact on the bitstream. Reduced to one Plane( p ) call. LumaLine(), CbLine(), CrLine() and AlphaLine() are actually same; the order of lines is already defined in the General section and has no impact on the bitstream. Reduced to one Line( p, y ) call. plane_count name may be misleading (it is the count of quant_table_index, which is not always the count of planes) and does not exist in the bistream, replaced by the sum of existing bitstream elements. colorspace_type related if sorted in ascending order. --- ffv1.lyx | 801 +-- 1 file changed, 53 insertions(+), 748 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index dd5eb50..07e9348 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -2897,7 +2897,7 @@ Slice \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=27 columns=2 +lyxtabular version=3 rows=18 columns=2 features rotate=0 tabularvalignment=middle column
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Reduce redundancy in the description of xxPlane() and xxLine()
Le 03/05/2015 13:55, Michael Niedermayer a écrit : On Sun, May 03, 2015 at 12:31:05PM +0200, Jerome Martinez wrote: - plane_count which is the count of planes thats a bad name for packed formats Good point, but this is not worse than the previous name used ;-). and actually you use alpha_plane even for color_space = 1 (so no planes) Attached is a new patch with channel_count name. channel wording taken from https://www.ffmpeg.org/ffmpeg-all.html#extractplanes . should I also change, in another patch, chroma_planes and alpha_plane to chroma_channels and alpha_channel for more coherency? - quant_table_index_count which is the count of quant_table indexes any reason not to call it quant_table_count ? (its shorter and seems not to contain less information) quant_table_count is already defined as the count of quantization tables (in the header), we can not reuse that name for something different. here, it is the count of quantization table indexes, which is different (it is possible to have 1000 quantization tables, but we can have only up to 3 quantization indexes in the slice) if I well understood. I would like to have something shorter too, but I preferred not to modify quant_table_index[i][j] quant_table_count is the count of quant_tables quant_table_index_count is the count of quant_table_index. please keep the line length of commit messages below 80 but yes ive missed that part of the commit message Ho, I was not aware of this limitation (I have carriage returns in my tools). Modified to 50/72 best practice as seen in different places. It is not good to have redundancy in a specification. Its more important that a spec is readable than being non redundant its after all a english text that has to be understood by a human I kindly disagree: we need a good balance between redundancy and readability. Here, I think the redundancy is not useful. I plan to propose in the future a patch for a better readability of the order of planes. [...] From 9f4b73e4153b6c4829d814da15bf22695f1e3df9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Sun, 3 May 2015 14:57:27 +0200 Subject: [PATCH] Reduce redundancy in xxPlane() and xxLine(). LumaPlane(), CbPlane(), CrPlane() and AlphaPlane() are actually same; the order of planes is already defined in the General section and has no impact on the bitstream. Reduced to one Plane( p ) call. LumaLine(), CbLine(), CrLine() and AlphaLine() are actually same; the order of lines is already defined in the General section and has no impact on the bitstream. Reduced to one Line( p, y ) call. plane_count name may be misleading (it is the count of quant_table_index, which is not always the count of planes) and does not exist in the bistream, replaced by the sum of existing bitstream elements. colorspace_type related if sorted in ascending order. --- ffv1.lyx | 801 +-- 1 file changed, 53 insertions(+), 748 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index dd5eb50..e29f5e1 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -2897,7 +2897,7 @@ Slice \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=27 columns=2 +lyxtabular version=3 rows=18 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -3008,537 +3008,6 @@ SliceHeader( i ) /cell /row row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -if( colorspace_type == 1) { -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -for( y = 0; -\begin_inset space ~ -\end_inset - -y height; -\begin_inset space ~ -\end_inset - -y++ ) { -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=center valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset
[FFmpeg-devel] [PATCH] ffv1dec: plane_index is 1 in case of version 4 gray+alpha.
Le 02/05/2015 12:51, Michael Niedermayer a écrit : On Sat, May 02, 2015 at 02:41:33AM +0200, Jerome Martinez wrote: shouldn't it be if (fs-transparency) decode_plane(fs, p-data[3] + ps*x + y*p-linesize[3], width, height, p-linesize[3], (f-version = 4 !f-chroma_planes) ? 1 : 2); yes, i think so too, please send a patch, Attached note though this isnt a bug as the case where this matters isnt supported Sure (version 4 not considered stable). From 0847ef91514a1a05ae1a09e170f470a884f74283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Sat, 2 May 2015 13:23:12 +0200 Subject: [PATCH] ffv1dec: plane_index is 1 in case of version 4 gray+alpha. Since version 4, plane_index for the alpha plane is 1 in the case chroma_planes is 0. --- libavcodec/ffv1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index fda3f09..3877768 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -441,7 +441,7 @@ static int decode_slice(AVCodecContext *c, void *arg) decode_plane(fs, p-data[2] + ps*cx+cy*p-linesize[2], chroma_width, chroma_height, p-linesize[2], 1); } if (fs-transparency) -decode_plane(fs, p-data[3] + ps*x + y*p-linesize[3], width, height, p-linesize[3], 2); +decode_plane(fs, p-data[3] + ps*x + y*p-linesize[3], width, height, p-linesize[3], f-version = 4 !f-chroma_planes) ? 1 : 2); } else { uint8_t *planes[3] = { p-data[0] + ps * x + y * p-linesize[0], p-data[1] + ps * x + y * p-linesize[1], -- 1.9.5.msysgit.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Reduce redundancy in the description of xxPlane() and xxLine()
New patch because I misunderstood the definition of plane_count. Now LyX Document is 1 + ( ( chroma_planes || version 4 ) ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ) Le 02/05/2015 01:33, Jerome Martinez a écrit : Some notes: - I discarded the if version = 4 stuff for grayscale because I don't see such limitation in the bitstream and in the source code. I am thinking to add a specific section about decoder limitations (e.g. bits_per_raw_sample accepted range, gray/alpha support...) - I hesitated to define * quant_table_index_count = 1 + ( chroma_planes ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ) * plane_count = 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane ? 1 : 0 ) but they are nowhere else in the specification, so I direclty put these formulas in the if. I think it is a bit verbose in the if but has the advantage not to have to define extra parameters and we focus on the bitstream instead. - xxPlanes and xxLines were never defined, so I replace undefined functions by other undefined functions. Planes and Lines functions will be defined in future patches. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] FFV1 specification: Reduce redundancy in the description of xxPlane() and xxLine()
New patch because I misunderstood the definition of plane_count. Now is 1 + ( ( chroma_planes || version 4 ) ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ) Le 02/05/2015 01:33, Jerome Martinez a écrit : Some notes: - I discarded the if version = 4 stuff for grayscale because I don't see such limitation in the bitstream and in the source code. I am thinking to add a specific section about decoder limitations (e.g. bits_per_raw_sample accepted range, gray/alpha support...) - I hesitated to define * quant_table_index_count = 1 + ( chroma_planes ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ) * plane_count = 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane ? 1 : 0 ) but they are nowhere else in the specification, so I direclty put these formulas in the if. I think it is a bit verbose in the if but has the advantage not to have to define extra parameters and we focus on the bitstream instead. - xxPlanes and xxLines were never defined, so I replace undefined functions by other undefined functions. Planes and Lines functions will be defined in future patches. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From a70a7132fe56a144a668736cc7864d9dd232d818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Sat, 2 May 2015 01:53:47 +0200 Subject: [PATCH] Reduce redundancy in the description of xxPlane() and xxLine(). LumaPlane(), CbPlane(), CrPlane() and AlphaPlane() are actually same; the order of planes is already defined in the General section and has no impact on the bitstream. Reduced to one Plane( p ) call. LumaLine(), CbLine(), CrLine() and AlphaLine() are actually same; the order of lines is already defined in the General section and has no impact on the bitstream. Reduced to one Line( p, y ) call. plane_count name may be misleading (it is the count of quant_table_index, which is not always the count of planes) and does not exist in the bistream, replaced by the sum of existing bitstream elements. colorspace_type related if sorted in ascending order. --- ffv1.lyx | 794 --- 1 file changed, 46 insertions(+), 748 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index dd5eb50..98cc5bb 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -2897,7 +2897,7 @@ Slice \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=27 columns=2 +lyxtabular version=3 rows=18 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -3008,537 +3008,6 @@ SliceHeader( i ) /cell /row row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -if( colorspace_type == 1) { -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -for( y = 0; -\begin_inset space ~ -\end_inset - -y height; -\begin_inset space ~ -\end_inset - -y++ ) { -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=center valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -LumaLine( y ) -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=center valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space
Re: [FFmpeg-devel] FFV1 specification: plane_count prior to version 4
Le 02/05/2015 01:37, Michael Niedermayer a écrit : plane_count was 2 for gray prior to version 4 [...] Got it. Maybe I missed something else, but looks like decode_plane() is never called with plane_index = 1 if chroma_planes is 0. Was it an implementation bug and actually plane_index = 1 is never used in the case of gray? Additionally, I read this code: if (fs-transparency) decode_plane(fs, p-data[3] + ps*x + y*p-linesize[3], width, height, p-linesize[3], 2); plane_index is 2 for the alpha plane if transparency is 1, in all cases. but I understood that for version = 4, plane_count is 2 if grey+alpha, so plane_index = 2 would not be possible. shouldn't it be if (fs-transparency) decode_plane(fs, p-data[3] + ps*x + y*p-linesize[3], width, height, p-linesize[3], (f-version = 4 !f-chroma_planes) ? 1 : 2); ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] FFV1 specification: Add missing { }
Le 02/05/2015 00:21, Michael Niedermayer a écrit : why the extra space ? Because of a guy doing stupid mistakes. (this guy may try to blame Lyx but it is difficult to be believable) New patch attached. From 40a211a344c3f7bcb8bc627bd5edcb4cb8b1c56c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Fri, 1 May 2015 14:22:06 +0200 Subject: [PATCH 2/4] Add missing { } --- ffv1.lyx | 39 +-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index c30ad67..6608ccf 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -3972,7 +3972,7 @@ Slice Header \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=14 columns=2 +lyxtabular version=3 rows=15 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -4347,7 +4347,7 @@ ur \begin_inset space ~ \end_inset -if( version 3 ) +if( version 3 ) { \end_layout \end_inset @@ -4465,6 +4465,41 @@ ur /cell /row row +cell alignment=center valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +} +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout + +\end_layout + +\end_inset +/cell +/row +row cell alignment=center valignment=top topline=true bottomline=true leftline=true usebox=none \begin_inset Text -- 1.9.5.msysgit.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/4] FFV1 specification: Add missing { }
Le 02/05/2015 01:05, Michael Niedermayer a écrit : this looks wrong in my lyx, quite wrong, certainly doesnt result in a closing bracket Weird, it is OK on my side and actually a pretty patch. I rebased following your latest commits, attached is a new patch. note, that is when i apply the patch, as you didnt push it to your tree Push request up to date now. Thank you for your patience. From d60fac29a32c37620e6b49725c510cb13c915a7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Fri, 1 May 2015 14:22:06 +0200 Subject: [PATCH] Add missing { } --- ffv1.lyx | 39 +-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index dd5eb50..05f4863 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -4555,7 +4555,7 @@ Slice Header \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=14 columns=2 +lyxtabular version=3 rows=15 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -4930,7 +4930,7 @@ ur \begin_inset space ~ \end_inset -if( version 3 ) +if( version 3 ) { \end_layout \end_inset @@ -5048,6 +5048,41 @@ ur /cell /row row +cell alignment=center valignment=top topline=true leftline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + + +\begin_inset space ~ +\end_inset + +} +\end_layout + +\end_inset +/cell +cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none +\begin_inset Text + +\begin_layout Plain Layout + +\end_layout + +\end_inset +/cell +/row +row cell alignment=center valignment=top topline=true bottomline=true leftline=true usebox=none \begin_inset Text -- 1.9.5.msysgit.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] FFV1 specification: Reduce redundancy in the description of xxPlane() and xxLine()
Some notes: - I discarded the if version = 4 stuff for grayscale because I don't see such limitation in the bitstream and in the source code. I am thinking to add a specific section about decoder limitations (e.g. bits_per_raw_sample accepted range, gray/alpha support...) - I hesitated to define * quant_table_index_count = 1 + ( chroma_planes ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ) * plane_count = 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane ? 1 : 0 ) but they are nowhere else in the specification, so I direclty put these formulas in the if. I think it is a bit verbose in the if but has the advantage not to have to define extra parameters and we focus on the bitstream instead. - xxPlanes and xxLines were never defined, so I replace undefined functions by other undefined functions. Planes and Lines functions will be defined in future patches. From 3a45b76fc75e3caa82fb38680e653c17ea0c8ad1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Sat, 2 May 2015 01:25:52 +0200 Subject: [PATCH] Reduce redundancy in the description of xxPlane() and xxLine(). LumaPlane(), CbPlane(), CrPlane() and AlphaPlane() are actually same; the order of planes is already defined in the General section and has no impact on the bitstream. Reduced to one Plane( p ) call. LumaLine(), CbLine(), CrLine() and AlphaLine() are actually same; the order of lines is already defined in the General section and has no impact on the bitstream. Reduced to one Line( p, y ) call. plane_count name may be misleading (it is the count of quant_table_index, which is not always the count of planes) and does not exist in the bistream, replaced by the sum of existing bitstream elements. colorspace_type related if sorted in ascending order. --- ffv1.lyx | 794 --- 1 file changed, 46 insertions(+), 748 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index dd5eb50..e4da856 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -2897,7 +2897,7 @@ Slice \begin_layout Standard \begin_inset Tabular -lyxtabular version=3 rows=27 columns=2 +lyxtabular version=3 rows=18 columns=2 features rotate=0 tabularvalignment=middle column alignment=left valignment=top column alignment=center valignment=top @@ -3008,537 +3008,6 @@ SliceHeader( i ) /cell /row row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -if( colorspace_type == 1) { -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=left valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -for( y = 0; -\begin_inset space ~ -\end_inset - -y height; -\begin_inset space ~ -\end_inset - -y++ ) { -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=center valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - -LumaLine( y ) -\end_layout - -\end_inset -/cell -cell alignment=center valignment=top topline=true leftline=true rightline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout - -\end_layout - -\end_inset -/cell -/row -row -cell alignment=center valignment=top topline=true leftline=true usebox=none -\begin_inset Text - -\begin_layout Plain Layout -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset - - -\begin_inset space ~ -\end_inset
[FFmpeg-devel] [PATCH 1/4] FFV1 specification: Change formatting of pseudo-code
From 9da73eaa1ddf40667dca300ee89430283c1e36ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= jer...@mediaarea.net Date: Fri, 1 May 2015 10:54:04 +0200 Subject: [PATCH 1/4] Change formatting of pseudo-code. In order to have the same formatting everywhere. Formatting is the same as in some ISO or ITU specifications. --- ffv1.lyx | 136 +++ 1 file changed, 68 insertions(+), 68 deletions(-) diff --git a/ffv1.lyx b/ffv1.lyx index d4911cd..2942c10 100644 --- a/ffv1.lyx +++ b/ffv1.lyx @@ -1957,7 +1957,7 @@ Frame \begin_inset Text \begin_layout Plain Layout -Frame { +Frame( ) { \end_layout \end_inset @@ -2027,7 +2027,7 @@ br \begin_inset space ~ \end_inset -if (keyframe) { +if ( keyframe ) { \end_layout \end_inset @@ -2078,7 +2078,7 @@ if (keyframe) { \begin_inset space ~ \end_inset -if(version2) +if( version 2 ) \end_layout \end_inset @@ -2145,7 +2145,7 @@ if(version2) \begin_inset space ~ \end_inset -FrameHeader01 +FrameHeader01( ) \end_layout \end_inset @@ -2215,7 +2215,7 @@ FrameHeader01 \begin_inset space ~ \end_inset -for(i=0; islice_count; i++) +for( i = 0; i slice_count; i++) \end_layout \end_inset @@ -2266,7 +2266,7 @@ for(i=0; islice_count; i++) \begin_inset space ~ \end_inset -Slice(i) +Slice( i ) \end_layout \end_inset @@ -2323,7 +2323,7 @@ Slice \begin_inset Text \begin_layout Plain Layout -Slice(i) { +Slice( i ) { \end_layout \end_inset @@ -2358,7 +2358,7 @@ type \begin_inset space ~ \end_inset -if(version2) +if( version 2 ) \end_layout \end_inset @@ -2409,7 +2409,7 @@ if(version2) \begin_inset space ~ \end_inset -SliceHeader(i) +SliceHeader( i ) \end_layout \end_inset @@ -2444,7 +2444,7 @@ SliceHeader(i) \begin_inset space ~ \end_inset -if (colorspace_type == 1) { +if( colorspace_type == 1) { \end_layout \end_inset @@ -2495,15 +2495,15 @@ if (colorspace_type == 1) { \begin_inset space ~ \end_inset -for (y=0; +for ( y = 0; \begin_inset space ~ \end_inset -yheight; +y height; \begin_inset space ~ \end_inset -y++) { +y++ ) { \end_layout \end_inset @@ -2570,7 +2570,7 @@ y++) { \begin_inset space ~ \end_inset -LumaLine[y] +LumaLine( y ) \end_layout \end_inset @@ -2637,7 +2637,7 @@ LumaLine[y] \begin_inset space ~ \end_inset -CbLine[y] +CbLine( y ) \end_layout \end_inset @@ -2704,7 +2704,7 @@ CbLine[y] \begin_inset space ~ \end_inset -CrLine[y] +CrLine( y ) \end_layout \end_inset @@ -2771,7 +2771,7 @@ CrLine[y] \begin_inset space ~ \end_inset -if (alpha_plane) +if ( alpha_plane ) \end_layout \end_inset @@ -2854,7 +2854,7 @@ if (alpha_plane) \begin_inset space ~ \end_inset -AlphaLine[y] +AlphaLine( y ) \end_layout \end_inset @@ -2991,7 +2991,7 @@ AlphaLine[y] \begin_inset space ~ \end_inset -LumaPlane +LumaPlane( ) \end_layout \end_inset @@ -3042,7 +3042,7 @@ LumaPlane \begin_inset space ~ \end_inset -if (chroma_planes) { +if ( chroma_planes ) { \end_layout \end_inset @@ -3109,7 +3109,7 @@ if (chroma_planes) { \begin_inset space ~ \end_inset -CbPlane +CbPlane( ) \end_layout \end_inset @@ -3176,7 +3176,7 @@ CbPlane \begin_inset space ~ \end_inset -CrPlane +CrPlane( ) \end_layout \end_inset @@ -3278,7 +3278,7 @@ CrPlane \begin_inset space ~ \end_inset -if (alpha_plane) +if ( alpha_plane ) \end_layout \end_inset @@ -3345,7 +3345,7 @@ if (alpha_plane) \begin_inset space ~ \end_inset -AlphaPlane +AlphaPlane( ) \end_layout \end_inset @@ -3415,7 +3415,7 @@ AlphaPlane \begin_inset space ~ \end_inset -if(i || version2) +if( i || version 2 ) \end_layout \end_inset @@ -3501,7 +3501,7 @@ u(24) \begin_inset space ~ \end_inset -if(ec){ +if( ec ) { \end_layout \end_inset @@ -3981,7 +3981,7 @@ Slice Header \begin_inset Text \begin_layout Plain Layout -SliceHeader(i) { +SliceHeader( i ) { \end_layout \end_inset @@ -4086,7 +4086,7 @@ ur \begin_inset space ~ \end_inset -slice_width-1 +slice_width - 1 \end_layout \end_inset @@ -4121,7 +4121,7 @@ ur \begin_inset space ~ \end_inset -slice_height-1 +slice_height - 1 \end_layout \end_inset @@ -4156,7 +4156,7 @@ ur \begin_inset space ~ \end_inset -for(j=0; jplane_count; j++) +for( j = 0; j plane_count; j++) \end_layout \end_inset @@ -4347,7 +4347,7 @@ ur \begin_inset space ~ \end_inset -if (version 3) +if ( version 3 ) \end_layout \end_inset @@ -4827,7 +4827,7 @@ Version 0 and 1 \begin_inset Text \begin_layout Plain Layout -FrameHeader01 { +FrameHeader01( ) { \end_layout \end_inset @@ -4932,7 +4932,7 @@ ur \begin_inset space ~ \end_inset -if(coder_type1) +if( coder_type 1 ) \end_layout \end_inset @@ -4983,7 +4983,7 @@ if(coder_type1) \begin_inset space ~ \end_inset -for(i=1; i256; i++) +for( i = 1; i 256; i++ ) \end_layout \end_inset @@ -5120,7 +5120,7 @@ ur \begin_inset space ~ \end_inset -if(version0) +if( version 0
[FFmpeg-devel] [PATCH 0/4] FFV1 specification: Add formating and conventions
The purpose of this set of patches is to have a coherent document with explicit meaning of used operators. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/4] FFV1 specification: Add missing { }
___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/4] FFV1 specification: Add conventions
___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel